feat!: add package policy management contract - #99
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds cross-language package-policy management contracts, including draft validation, optimistic replacement, management snapshots, and JSON-only policy models.
Changes:
- Adds Rust and .NET management APIs, routes, DTOs, validation, and clients.
- Introduces editable policy drafts and removes YAML policy parsing.
- Adds fixtures and stricter boolean-match validation.
Reviewed changes
Copilot reviewed 52 out of 53 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
policies/test-data/package-broker/scenarios/baseline.scenarios.json |
Removes YAML-policy scenarios. |
policies/test-data/package-broker/responses/policy-validation.valid.response.json |
Adds valid-validation fixture. |
policies/test-data/package-broker/responses/policy-validation.invalid.response.json |
Adds invalid-validation fixture. |
policies/test-data/package-broker/responses/policy-stale-token.error.json |
Adds stale-token error fixture. |
policies/test-data/package-broker/responses/policy-replacement.response.json |
Adds replacement response fixture. |
policies/test-data/package-broker/responses/policy-management.missing.response.json |
Adds missing-policy snapshot. |
policies/test-data/package-broker/responses/policy-management.invalid.response.json |
Adds invalid-policy snapshot. |
policies/test-data/package-broker/responses/policy-management.active.response.json |
Adds active-policy snapshot. |
policies/test-data/package-broker/requests/policy-validation.request.json |
Adds raw validation request. |
policies/test-data/package-broker/requests/policy-replacement.update.request.json |
Adds update intent fixture. |
policies/test-data/package-broker/requests/policy-replacement.replace-identity.request.json |
Adds identity-replacement fixture. |
policies/test-data/package-broker/requests/policy-replacement.repair.request.json |
Adds repair intent fixture. |
policies/test-data/package-broker/requests/policy-replacement.overwrite.request.json |
Adds confirmed-overwrite fixture. |
policies/test-data/package-broker/requests/policy-replacement.create.request.json |
Adds create intent fixture. |
policies/rust/now-policy/tests/policy_samples.rs |
Tests drafts and boolean matches. |
policies/rust/now-policy/src/schema.rs |
Adds draft-schema generation. |
policies/rust/now-policy/src/policy.rs |
Adds draft models and validation. |
policies/rust/now-policy/schema/devolutions.now-policy.schema.json |
Regenerates policy schema. |
policies/rust/now-policy/README.md |
Documents JSON-only drafts. |
policies/rust/now-policy/CHANGELOG.md |
Records model changes. |
policies/rust/now-policy/Cargo.toml |
Removes YAML dependency. |
policies/rust/now-policy/assets/samples/corporate-allowlist.policy.yaml |
Removes YAML sample. |
policies/rust/now-policy-server-template/tests/support/mock.rs |
Extends server mock. |
policies/rust/now-policy-server-template/tests/sample_documents.rs |
Tests routes and fixtures. |
policies/rust/now-policy-server-template/src/server.rs |
Adds management routes and mappings. |
policies/rust/now-policy-server-template/README.md |
Documents server endpoints. |
policies/rust/now-policy-server-template/CHANGELOG.md |
Records server contract changes. |
policies/rust/now-policy-api/src/policy.rs |
Adds draft schema reference. |
policies/rust/now-policy-api/src/management.rs |
Defines management contracts. |
policies/rust/now-policy-api/src/lib.rs |
Exports management models and markers. |
policies/rust/now-policy-api/src/enums.rs |
Adds management error codes. |
policies/rust/now-policy-api/src/api.rs |
Adds validation to errors. |
policies/rust/now-policy-api/README.md |
Documents management architecture. |
policies/rust/now-policy-api/openapi/now-policy-api.yaml |
Adds generated management OpenAPI. |
policies/rust/now-policy-api/CHANGELOG.md |
Records API additions. |
policies/dotnet/Devolutions.Now.Policy.Model/README.md |
Documents .NET draft model. |
policies/dotnet/Devolutions.Now.Policy.Model/PolicyModels.cs |
Adds draft conversion models. |
policies/dotnet/Devolutions.Now.Policy.Model/PolicyJson.cs |
Adds draft and boolean validation. |
policies/dotnet/Devolutions.Now.Policy.Model/Devolutions.Now.Policy.Model.csproj |
Removes YamlDotNet. |
policies/dotnet/Devolutions.Now.Policy.Model.Tests/PolicyTests.cs |
Tests draft conversions. |
policies/dotnet/Devolutions.Now.Policy.Client/README.md |
Documents client methods. |
policies/dotnet/Devolutions.Now.Policy.Client/BrokerClient.cs |
Implements management client APIs. |
policies/dotnet/Devolutions.Now.Policy.Client.Tests/TestData.cs |
Classifies new fixtures. |
policies/dotnet/Devolutions.Now.Policy.Client.Tests/PolicyManagementClientTests.cs |
Tests management client behavior. |
policies/dotnet/Devolutions.Now.Policy.Client.Tests/MetaModelTests.cs |
Verifies source-generated DTOs. |
policies/dotnet/Devolutions.Now.Policy.Client.Tests/DtoRoundTripTests.cs |
Adds contract round-trip tests. |
policies/dotnet/Devolutions.Now.Policy.Api/README.md |
Documents management DTOs. |
policies/dotnet/Devolutions.Now.Policy.Api/PolicyManagementModels.cs |
Defines .NET management DTOs. |
policies/dotnet/Devolutions.Now.Policy.Api/MetaModels.cs |
Extends structured errors. |
policies/dotnet/Devolutions.Now.Policy.Api/Enums.cs |
Adds strict management errors. |
policies/dotnet/Devolutions.Now.Policy.Api/BrokerJson.cs |
Adds management serialization contexts. |
policies/dotnet/Devolutions.Now.Policy.Api/BrokerApi.cs |
Adds protocol discriminators. |
Cargo.lock |
Removes Rust YAML dependency. |
Suppressed comments (1)
policies/dotnet/Devolutions.Now.Policy.Api/BrokerJson.cs:119
- This validation does not reject null entries in
PolicyValidationResult.Findings.RespectNullableAnnotationsdoes not enforce collection-element nullability, so a successful response or error containing"Findings":[null]is accepted by the .NET client while Rust rejects it, and callers can then fail when reading a finding. Explicitly reject null finding elements here.
private static void ValidateValidation(PolicyValidationResult validation)
{
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Require atomic stale-token snapshots, enforce validation and management invariants, preserve legacy route 404s, and map unsafe paths to HTTP 409. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Apply semantic invariants to both C# deserialization modes, restrict opaque values to safe ASCII, and preserve nullable optional schemas without weakening required states. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Add the separate 16 MiB policy-management body limit, align Unicode text bounds, and distinguish unsupported non-JSON policy paths. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Validate direct policy match DTOs and nested management findings, and clarify boolean match cardinality diagnostics. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Replace format-named .NET serializer helpers with responsibility-based names and make the lossy Rust policy-to-draft projection explicit. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Serialization invariants, revision bounds, and replacement OpenAPI responses remain inconsistent with the declared contract.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 64/65 changed files
- Comments generated: 5
- Review effort level: Balanced
Design record: decisions and API-boundary rationaleDuring the Phase 2 design and review passes, I made the following contract decisions. This comment records the intent behind them so that later implementation work does not have to infer semantics from DTO shapes alone. Decisions made during review
API surface changes and rationalePolicy model
HTTP management boundary
Validation boundary
Replacement boundary
Error boundary
Rust server boundary
.NET public boundary
OpenAPI and fixtures
The resulting boundary intentionally separates four concepts: the committed active policy, the editable draft, the atomic store/management observation, and the authoritative validate-and-commit workflow. |
Validate Rust boolean-match serialization, enforce shared revision limits, apply semantic checks through public .NET serializer options, and document replacement media-type errors. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Replacement invariants and several public serialization validation paths remain unenforced.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
policies/dotnet/Devolutions.Now.Policy.Api/BrokerSerializer.cs:52
- The callbacks are attached to every generated object type, but this switch ignores the public
PolicyValidationResultandPolicyManagementSnapshotDTOs. SinceBrokerSerializer.Options.GetTypeInfoexplicitly exposes both types, directJsonSerializercalls on them still accept contradictory validity/state combinations; only wrapping them in a response triggers validation. Add direct cases so the public options enforce invariants for every supported DTO.
private static void ValidateSemanticValue(object? value)
{
switch (value)
{
case PolicyResponse response:
policies/dotnet/Devolutions.Now.Policy.Model/PolicySerializer.cs:14
- These public options bypass
ValidateSemanticValuebecause they expose the generated contexts directly. For example, serializing aPolicyMatchwithInteractive = [false, true]throughJsonSerializerandPolicySerializer.Optionsemits a document that the schema, Rust model, andPolicySerializer.Serializereject;StrictOptionssimilarly accepts it on input. Attach semantic callbacks to these option resolvers, or avoid exposing them as equivalent serializer entry points.
public static readonly JsonSerializerOptions Options = new(PolicySerializerContext.Default.Options)
{
};
public static readonly JsonSerializerOptions StrictOptions = new(Options)
- Files reviewed: 64/65 changed files
- Comments generated: 3
- Review effort level: Balanced
Reject impossible replacement successes, validate committed revisions and direct serializer option paths, and align the generated OpenAPI refinements. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Draft schema identity, validation fixture binding, and package-route OpenAPI responses remain inconsistent.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
policies/test-data/package-broker/requests/policy-validation.request.json:17
- This submitted draft has no rules, but the paired valid response's
CanonicalDraftaddsallow.vscode.skip-hashand issues aSensitiveOptionAllowedwarning for it. That makes the fixture model a receipt and findings bound to content that was never submitted. Add the rule here, or remove it and its warning from the response, so the validation round-trip exercises the advertised binding.
- Files reviewed: 64/65 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.
🔵 Needs a closer look
Public broker serializer options bypass semantic validation for directly serialized policy-model roots.
Review details
Suppressed comments (1)
policies/dotnet/Devolutions.Now.Policy.Api/BrokerSerializer.cs:74
- The public resolver also exposes
PolicyDocumentandPolicyDraftDocument(and the tests explicitly treat them as supported DTOs), but this callback has no case for either type. A directJsonSerializer.Serialize/Deserialize(..., BrokerSerializer.Options)therefore bypassesPolicySerializervalidation and can accept or emit a wrong$schema, revision0, or multi-value boolean match, while the same document is rejected when embedded inPolicyResponse. Delegate policy-model roots toPolicySerializerhere, or stop exposing them through these public options.
private static void ValidateSemanticValue(object? value)
{
switch (value)
{
case PolicyResponse response:
PolicySerializer.ValidateRequiredCollectionElements(response.Policy);
break;
case PolicyManagementResponse response:
ValidateManagement(response.Management);
break;
case PolicyValidationResponse response:
ValidateValidation(response.Validation);
break;
case PolicyReplacementResponse response:
ValidateReplacement(response);
break;
case PolicyValidationResult validation:
ValidateValidation(validation);
break;
case PolicyManagementSnapshot management:
ValidateManagement(management);
break;
case ErrorResponse error:
ValidateError(error);
break;
}
- Files reviewed: 67/68 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Addressed the suppressed |
There was a problem hiding this comment.
🟡 Changes recommended
Replacement responses do not ensure their duplicated policy views represent the same committed document.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 67/68 changed files
- Comments generated: 2
- Review effort level: Balanced
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Disposition for Copilot's broad risk verdict: non-actionable because it identifies no concrete defect or remediation and generated no comments. The PR has human approval; exact head |
There was a problem hiding this comment.
🟡 Changes recommended
Direct .NET serialization of PolicyConstraints bypasses the newly introduced bounded-string validation.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 67/68 changed files
- Comments generated: 1
- Review effort level: Balanced
| case PolicyMatch match: | ||
| ValidateRequiredCollectionElements(match, "$"); | ||
| break; |
- validate standalone `PolicyConstraints` values through `PolicySerializer.Serialize<T>` and `DeserializeStrict<T>` - reuse the same four bounded-string checks used for constraints nested in policy rules - enforce the checks through direct `JsonSerializer` use with public `PolicySerializer.Options` and `StrictOptions` - cover all four collections at empty, maximum, and maximum+1 Unicode-scalar lengths across generic helpers and public options This is an immediate follow-up to merged #99 for the late [Copilot review](#99 (review)) and its [specific finding](#99 (comment)). `BrokerSerializer` needs no additional change: its supported generic APIs expose broker DTO roots, whose embedded policy documents already traverse `PolicySerializer` validation. It does not expose standalone `PolicyConstraints` through its generic helper surface. Changelog: ignore Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Add a versioned package-policy management contract alongside the unchanged active-policy inspection API. New management, validation, and replacement endpoints expose atomic Active/Missing/Invalid snapshots, raw-draft authoritative validation with structured findings and warning-bound receipts, and exact-token optimistic replacement with explicit Update, ReplaceIdentity, Create, and Repair intents. Rust server implementations gain the corresponding required trait methods and routes, while .NET gains NativeAOT-safe DTOs and cancellation-aware client APIs.
Make policy documents JSON-only, removing Rust
parse_policy_yamland .NETPolicyDocument.ParseYaml. IntroducePolicyDraftDocumentfor authored policy content without server-managed revision and publication metadata, with its own versioned JSON Schema identity and explicit named conversions to and from committed policies. Rename the public .NET serialization helpers toPolicySerializerandBrokerSerializer, and replace Rust’s lossyFrom<&PolicyDocument>draft projection withPolicyDocument::to_draft().Tighten cross-language contract validation for boolean matches, revision bounds, Unicode text lengths, opaque ASCII tokens and receipts, validation results, management snapshots, stale-token errors, and nullable schema fields. Unsafe paths and stale state use conflict semantics, unsupported policy formats and filesystems use unprocessable-entity semantics, and absent newer routes remain ordinary 404 responses. Validation and replacement accept complete HTTP request bodies up to 16 MiB through public Rust and .NET constants, while package-operation limits remain unchanged.