Skip to content

feat!: add package policy management contract - #99

Merged
Benoît Cortier (CBenoit) merged 11 commits into
masterfrom
cbenoit-phase-2-policy-contract
Sep 3, 2026
Merged

feat!: add package policy management contract#99
Benoît Cortier (CBenoit) merged 11 commits into
masterfrom
cbenoit-phase-2-policy-contract

Conversation

@CBenoit

@CBenoit Benoît Cortier (CBenoit) commented Aug 28, 2026

Copy link
Copy Markdown
Member

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_yaml and .NET PolicyDocument.ParseYaml. Introduce PolicyDraftDocument for 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 to PolicySerializer and BrokerSerializer, and replace Rust’s lossy From<&PolicyDocument> draft projection with PolicyDocument::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.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings August 28, 2026 17:53

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. RespectNullableAnnotations does 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.

Comment thread policies/dotnet/Devolutions.Now.Policy.Model/PolicySerializer.cs
Comment thread policies/dotnet/Devolutions.Now.Policy.Api/BrokerSerializer.cs
Comment thread policies/dotnet/Devolutions.Now.Policy.Model/PolicySerializer.cs
Comment thread policies/rust/now-policy/src/policy.rs
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>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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

Comment thread policies/rust/now-policy/src/policy.rs
Comment thread policies/dotnet/Devolutions.Now.Policy.Api/BrokerSerializer.cs
Comment thread policies/dotnet/Devolutions.Now.Policy.Model/PolicyModels.cs Outdated
Comment thread policies/rust/now-policy/src/policy.rs Outdated
Comment thread policies/rust/now-policy-server-template/src/server.rs
@CBenoit

Copy link
Copy Markdown
Member Author

Design record: decisions and API-boundary rationale

During 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

  1. Preserve Phase 1 inspection. GET /v1/policy remains the unconditional active-policy inspection endpoint with the existing PolicyResponse. Management is additive and does not redefine inspection behavior.

  2. Use one policy representation: JSON. Policy-specific YAML parsing is removed from Rust and .NET, including the previously public .NET PolicyDocument.ParseYaml. This is an intentional breaking change. OpenAPI may still be emitted as YAML, and unrelated YAML usage is unaffected. A configured non-JSON policy path is reported explicitly as unsupported format rather than being mislabeled unsafe.

  3. Separate authored content from a committed policy. Editors submit PolicyDraftDocument; they do not assign Revision or PublishedAt. The draft retains $schema, PolicyVersion, and PolicyType because those describe the policy language being authored, not the server-side publication history. The client may author a supported language/schema version, but the Agent remains authoritative and rejects unsupported or inconsistent values.

  4. Make lossy projection explicit. Converting a committed policy to a draft intentionally drops commit metadata, so Rust now uses the named PolicyDocument::to_draft() method rather than a generic From<&PolicyDocument> conversion. Draft-to-committed conversion is also named and requires the server-assigned revision and publication time.

  5. Name serializers by responsibility, not the only supported format. With JSON now being the sole policy/wire representation, the public .NET helpers are PolicySerializer and BrokerSerializer, replacing PolicyJson and BrokerJson. Their responsibility is source-generated serialization plus semantic contract validation; JSON no longer needs to be encoded in the public type name.

  6. Reject ineffective boolean matches. Boolean match arrays may be omitted, empty, [true], or [false]. More than one element is invalid, including duplicate values, because such states are either contradictory or semantically ineffective. Diagnostics say “at most one,” not “exactly one,” because an empty editor state remains valid.

  7. Return one atomic management view. A management read returns state, resolved path, exact opaque store token, configuration source, write guidance, stable read-only reason, elevation guidance, active content, and invalid diagnostics together. This avoids composing a UI from observations made at different store states.

  8. Treat capabilities as guidance only. WriteCapability, ReadOnlyReason, and ElevationRequired support UX decisions; they do not authorize a write. The Agent must repeat authorization, path, filesystem, token, parsing, and validation checks inside the transaction.

  9. Preserve raw drafts until authoritative validation. Validate/replace envelopes carry raw JSON (serde_json::Value / JsonElement) so an older client model cannot silently discard unknown fields before the implementation validator sees them. Malformed JSON may still be rejected by transport extraction.

  10. Make validation results self-consistent. IsValid=true requires a canonical typed draft and receipt and permits no Error findings; warnings are allowed. IsValid=false requires at least one Error and forbids canonical draft and receipt. Null finding elements are invalid. These rules apply to Rust deserialization/schema and both strict and non-strict C# semantic validation.

  11. Use stable structured findings. Findings carry severity, stable code, RFC 6901 pointer, optional rule ID, structured localization arguments, and fallback text. Initial warnings are intentionally narrow: audit mode, default allow, and enabled allow rules that may permit individually identified sensitive options. Evaluator-specific detection belongs to Gateway, while shapes and codes belong here.

  12. Bind warning confirmation to exact validated content. A validation receipt represents the canonical draft, validator version, and exact warning set. Changing the draft, validator behavior, or warnings invalidates the receipt; the Agent returns current findings rather than silently accepting stale confirmation.

  13. Make replacement intent explicit. The caller selects Update, ReplaceIdentity, Create, or Repair; the server does not infer identity semantics from a changed document. Update retains identity and increments revision. ReplaceIdentity intentionally replaces an active policy with a different logical ID and starts revision 1. Create handles a missing policy, and Repair replaces invalid configured content; both start revision 1. The Agent always assigns PublishedAt.

  14. Do not provide blind force. Every write carries the exact observed store token. ConfirmOverwrite means “overwrite the exact newly observed state represented by this token,” not “ignore concurrency.” Another intervening write conflicts again.

  15. Make stale conflicts retryable without a race. StalePolicyStoreToken errors include the atomic current PolicyManagementSnapshot. UniGetUI can display the newly observed state and, after confirmation, target its exact token without a follow-up GET that could race again.

  16. Reserve authentication statuses for authentication. HTTP 401 represents unauthenticated access and 403 represents administrator/elevation failure. Unsafe/read-only path state is a write/current-state conflict and maps to 409. Unsupported policy format and filesystem semantics map to 422.

  17. Preserve legacy endpoint detection. An older server that does not have a new route may return an ordinary unstructured 404. UnsupportedEndpoint exists for implementations that explicitly respond that way, but clients must not require it to detect absence.

  18. Use identical cross-language scalar contracts. Store tokens and receipts are implementation-generated opaque values, so they are restricted to bounded safe printable ASCII in Rust, C#, and schema rather than allowing UTF-8-byte versus UTF-16-code-unit drift. User-authored policy patterns, versions, and custom parameters instead use Unicode scalar/code-point counts, matching JSON Schema. Resource ID rules remain unchanged.

  19. Enforce semantic invariants on every supported deserialization path. C# strict mode differs from non-strict mode only in unknown-member handling. Both reject contradictory management snapshots, validation results, replacement responses, and management errors. Direct PolicyMatch and PolicyRule serialization/deserialization also enforce their own invariants.

  20. Apply a practical management transport cap. The complete HTTP body, including its envelope, for validation and replacement is limited to 16 MiB. This supports realistic policies up to the editor/model rule scale without promising the schema’s pathological theoretical maximum. Rust and C# expose the same constant. Package-operation endpoints retain their separate 256 KiB limit.

  21. Keep generated schemas honest. Nullable optional fields such as snapshot Policy and validation CanonicalDraft remain nullable at field level, while state/result oneOf branches require or forbid them. OpenAPI documents the 16 MiB operation extension, exact-case enums, and every runtime invariant that can be expressed structurally.

  22. Harden the final review findings without changing the wire shape. Direct policy-rule/match validation was completed, null findings were rejected in both validation and invalid-policy diagnostics, and boolean-array wording was corrected. These were behavioral rejection fixes for invalid values rather than new Gateway/UniGetUI DTO shapes.

  23. Keep rollout controlled. This repository is the first dependency for Gateway and UniGetUI. Packages are built as exact-head unpublished artifacts with provenance; nothing is published or merged by this work, and downstream PR readiness is not changed here.

API surface changes and rationale

Policy model

  • Removed: Rust parse_policy_yaml and .NET PolicyDocument.ParseYaml.

    • Why: avoid two parsing/coercion paths for one security-sensitive policy language.
  • Added: Rust/.NET PolicyDraftDocument and draft metadata.

    • Why: represent editable content without allowing clients to forge Revision or PublishedAt.
  • Added: Rust PolicyDocument::to_draft() and PolicyDraftDocument::into_policy_document(revision, published_at); .NET PolicyDocument.ToDraft() and PolicyDraftDocument.ToPolicyDocument(...).

    • Why: make the intentional removal/addition of commit metadata visible at the call site. Revision zero is rejected.
  • Added: Rust policy_draft_schema_json() and source-generated .NET support for draft serialization.

    • Why: let editors and consumers validate the editable shape directly rather than pretending it is a committed document.
  • Changed: boolean match collections accept at most one element.

    • Why: prevent contradictory or redundant matching semantics from reaching an evaluator.
  • Changed: StringPattern, VersionString, and CustomParameterString use Unicode scalar length bounds across Rust and .NET.

    • Why: match JSON Schema maxLength semantics and avoid cross-language rejection differences for multibyte text.

HTTP management boundary

  • Unchanged: GET /v1/policyPolicyResponse.

    • Why: active-policy inspection remains simple and backward compatible.
  • Added: GET /v1/policy/managementPolicyManagementResponse.

    • Why: administrative clients need a coherent store observation, not only a successfully parsed active policy.
  • Added: PolicyManagementSnapshot with State, ConfiguredPath, StoreToken, Source, WriteCapability, optional ReadOnlyReason, ElevationRequired, optional Policy, and optional InvalidDiagnostics.

    • Why: represent Active, Missing, and Invalid states atomically and provide stable UI guidance.
  • Added: PolicyManagementState, PolicyConfigurationSource, PolicyWriteCapability, and PolicyReadOnlyReason enums.

    • Why: replace message parsing with exact-case, versioned machine-readable state. Reasons include management disabled, path not configured, unsupported format, unsafe path, insufficient permissions, and unsupported filesystem.
  • Invariant: Active requires policy and forbids invalid diagnostics; Missing forbids both; Invalid forbids policy and requires nonempty diagnostics containing an Error. Writable forbids a read-only reason; ReadOnly/Unsupported require one.

    • Why: contradictory snapshots are not useful states and must fail at the boundary.

Validation boundary

  • Added: POST /v1/policy/validate with PolicyValidationRequest carrying fixed kind/version plus raw Draft JSON.

    • Why: preserve all representable client input for strict authoritative validation.
  • Added: PolicyValidationResponse / PolicyValidationResult with result version, validator version, validity, optional canonical draft, optional receipt, and findings.

    • Why: distinguish raw editor input from accepted canonical content and return reusable warning confirmation material.
  • Added: PolicyFinding, PolicyFindingSeverity, and PolicyFindingCode.

    • Why: give clients stable behavior/localization inputs while retaining fallback text for unknown codes.
  • Added deterministic codes: schema/unknown/missing/type/value errors; duplicate rule IDs; ineffective booleans; invalid or empty version ranges; invalid wildcards; contradictory constraints; invalid validity intervals; unsupported schema/type/version; and the deliberately narrow warning set.

    • Why: shared consumers need common vocabulary even though Gateway owns evaluator-specific detection.
  • Added: bounded safe-ASCII PolicyValidationReceipt.

    • Why: confirm exactly the canonical draft and warning set that the user reviewed.

Replacement boundary

  • Added: PUT /v1/policy with PolicyReplacementRequest carrying expected token, operation, conflict handling, warning acknowledgement, raw draft, and receipt.

    • Why: make identity, concurrency, validation, and warning intent explicit in one transaction request.
  • Added: PolicyReplacementOperation::{Update, ReplaceIdentity, Create, Repair}.

    • Why: each store state and identity transition has distinct revision semantics and should not be inferred.
  • Added: PolicyConflictHandling::{Reject, ConfirmOverwrite}.

    • Why: support deliberate retry against one exact newly observed state without introducing unconditional force.
  • Added: PolicyReplacementResponse containing the exact committed policy, transaction-time validation result, and new management snapshot/token.

    • Why: the response must describe precisely what became active after server assignment and atomic persistence.

Error boundary

  • Changed: ErrorResponse can carry Validation and Management; stale-token errors require Management.

    • Why: invalid/stale writes need current authoritative context, not only a string message.
  • Added management codes: UnsupportedEndpoint, MalformedDraft, InvalidPolicy, WarningConfirmationRequired, Unauthenticated, AdministratorRequired, UnsafePolicyPath, UnsupportedPolicyFormat, StalePolicyStoreToken, UnsupportedPolicyFilesystem, PolicyPersistenceFailed, and PolicyActivationFailed.

    • Why: distinguish transport, authentication, authorization, semantic, state, storage, persistence, and activation failures.
  • HTTP mapping: malformed draft 400; unauthenticated 401; administrator required 403; warning/path/token conflicts 409; body too large 413; unsupported media type 415; invalid policy/format/filesystem 422; persistence/activation 500; explicit unsupported endpoint 501.

    • Why: clients can choose remediation from status plus stable code, while 401/403 retain standard security meaning.

Rust server boundary

  • Added required trait methods: policy_management, validate_policy, and replace_policy.

    • Why: implementations provide domain/storage behavior behind a shared canonical router.
  • Added routes and mock support for management, validation, and replacement.

    • Why: runtime behavior, tests, and generated OpenAPI derive from the same server boundary.
  • Added: public MAX_POLICY_MANAGEMENT_BODY_BYTES = 16 * 1024 * 1024 applied only to POST validate and PUT replace.

    • Why: realistic management payloads require more room than package-operation requests, without weakening operation limits.

.NET public boundary

  • Renamed: PolicyJsonPolicySerializer; BrokerJsonBrokerSerializer, including public serializer options.

    • Why: name the responsibility rather than redundantly naming the sole representation.
  • Added client methods: GetPolicyManagement(CancellationToken), ValidatePolicy(JsonElement, CancellationToken), and ReplacePolicy(PolicyReplacementRequest, CancellationToken).

    • Why: expose cancellation-aware typed workflows while preserving raw draft JSON and strict response handling.
  • Added: BrokerApi.MaxPolicyManagementBodyBytes and management request/response kind constants.

    • Why: UniGetUI/helper framing can derive the same request maximum as the server, and message discriminators remain exact/versioned.
  • Changed: complete serialized UTF-8 validate/replace requests are preflighted against 16 MiB.

    • Why: fail predictably before transport and measure the same full-envelope unit as the server.
  • Changed: new DTOs and all supported serialization paths use source-generated System.Text.Json metadata and semantic dispatch.

    • Why: retain NativeAOT safety and prevent non-strict callers from accepting states Rust would reject.

OpenAPI and fixtures

  • Added: all management operations, components, status responses, exact-case enums, state/result branches, nullable optional fields, and x-max-request-body-bytes: 16777216 on validate/replace.

    • Why: generated documentation and schema consumers must observe the same contract as runtime code.
  • Added: shared positive and negative cross-language fixtures covering all management states, operations, findings, receipts, conflicts, stale snapshots, strict parsing, nulls, non-ASCII opaque values, and contradictory state/result combinations.

    • Why: prevent Rust, .NET, Gateway, and UniGetUI interpretations from drifting independently.

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>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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 PolicyValidationResult and PolicyManagementSnapshot DTOs. Since BrokerSerializer.Options.GetTypeInfo explicitly exposes both types, direct JsonSerializer calls 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 ValidateSemanticValue because they expose the generated contexts directly. For example, serializing a PolicyMatch with Interactive = [false, true] through JsonSerializer and PolicySerializer.Options emits a document that the schema, Rust model, and PolicySerializer.Serialize reject; StrictOptions similarly 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

Comment thread policies/rust/now-policy-api/src/management.rs
Comment thread policies/dotnet/Devolutions.Now.Policy.Model/PolicySerializer.cs Outdated
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>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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 CanonicalDraft adds allow.vscode.skip-hash and issues a SensitiveOptionAllowed warning 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

Comment thread policies/rust/now-policy/src/policy.rs Outdated
Comment thread policies/rust/now-policy-server-template/src/server.rs
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 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 PolicyDocument and PolicyDraftDocument (and the tests explicitly treat them as supported DTOs), but this callback has no case for either type. A direct JsonSerializer.Serialize/Deserialize(..., BrokerSerializer.Options) therefore bypasses PolicySerializer validation and can accept or emit a wrong $schema, revision 0, or multi-value boolean match, while the same document is rejected when embedded in PolicyResponse. Delegate policy-model roots to PolicySerializer here, 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>
@CBenoit

Copy link
Copy Markdown
Member Author

Addressed the suppressed Needs a closer look concern from #99 (review) in 27dcc5b. BrokerSerializer now delegates direct PolicyDocument and PolicyDraftDocument roots to the existing policy semantic validators, and focused tests exercise serialization and deserialization through both public option sets for schema, revision, and boolean-match violations. The review produced no inline thread, so there is no separate resolvable thread.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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

Comment thread policies/dotnet/Devolutions.Now.Policy.Api/BrokerSerializer.cs
Comment thread policies/rust/now-policy-api/src/management.rs
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Broad breaking contract and serialization changes across Rust and .NET warrant final human review.

Review details
  • Files reviewed: 67/68 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

@CBenoit

Copy link
Copy Markdown
Member Author

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 bdf8047 passed the full Windows/Linux/macOS CI matrix. Unpublished artifact version 2026.8.29-phase2.bdf80471 has provenance bound to that full commit and the recorded OpenAPI/committed-schema/draft-schema hashes.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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

Comment on lines +64 to +66
case PolicyMatch match:
ValidateRequiredCollectionElements(match, "$");
break;
@CBenoit
Benoît Cortier (CBenoit) merged commit cd7f3ba into master Sep 3, 2026
11 checks passed
@CBenoit
Benoît Cortier (CBenoit) deleted the cbenoit-phase-2-policy-contract branch September 3, 2026 15:58
Benoît Cortier (CBenoit) added a commit that referenced this pull request Sep 3, 2026
- 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants