feat(now-policy-api): add per-operation event channel protocol - #89
Conversation
There was a problem hiding this comment.
Pull request overview
Adds incremental polling for captured operation output across the Rust server/API and .NET client.
Changes:
- Defines output request/response wire models and OpenAPI schema.
- Adds server routing, mock support, and fixtures.
- Adds .NET client polling support and cross-platform tests.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
policies/rust/now-policy-api/src/output.rs |
Defines output polling models. |
policies/rust/now-policy-api/src/lib.rs |
Exports models and discriminators. |
policies/rust/now-policy-api/README.md |
Documents the new module. |
policies/rust/now-policy-api/openapi/now-policy-api.yaml |
Adds endpoint schemas. |
policies/rust/now-policy-server-template/src/server.rs |
Adds trait method and route. |
policies/rust/now-policy-server-template/src/mock.rs |
Adds mock output responses. |
policies/rust/now-policy-server-template/tests/sample_documents.rs |
Tests fixtures and dispatch. |
policies/rust/now-policy-server-template/assets/samples/requests/output-query-running.request.json |
Provides request fixture. |
policies/rust/now-policy-server-template/assets/samples/responses/output-chunk.response.json |
Provides running response fixture. |
policies/rust/now-policy-server-template/assets/samples/responses/output-eof.response.json |
Provides EOF response fixture. |
policies/dotnet/Devolutions.Now.Policy.Api/OutputModels.cs |
Adds .NET wire DTOs. |
policies/dotnet/Devolutions.Now.Policy.Api/BrokerApi.cs |
Adds constants and limits. |
policies/dotnet/Devolutions.Now.Policy.Api/BrokerJson.cs |
Registers JSON metadata. |
policies/dotnet/Devolutions.Now.Policy.Client/OperationOutputQuery.cs |
Adds client query model. |
policies/dotnet/Devolutions.Now.Policy.Client/BrokerClient.cs |
Implements output polling. |
policies/dotnet/Devolutions.Now.Policy.Client.Tests/TestData.cs |
Categorizes output fixtures. |
policies/dotnet/Devolutions.Now.Policy.Client.Tests/SchemaValidationTests.cs |
Validates output schemas. |
policies/dotnet/Devolutions.Now.Policy.Client.Tests/DtoRoundTripTests.cs |
Tests DTO round trips. |
policies/dotnet/Devolutions.Now.Policy.Client.Tests/BrokerClientTests.cs |
Tests client requests and clamping. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 30 changed files in this pull request and generated no new comments.
Suppressed comments (4)
policies/dotnet/Devolutions.Now.Policy.Client/OperationOutputQuery.cs:11
StreamKindis a required part of the wire request, but the client-facing query leaves it optional. Omitting it silently selects the enum's zero value (Stdout), so a caller can query the wrong stream without a compiler warning. Mark this memberrequired, as is done for the other mandatory client-facing enum fields.
public OutputStreamKind StreamKind { get; init; }
policies/dotnet/Devolutions.Now.Policy.Client/BrokerClient.cs:198
- This local variable uses a type-style PascalCase name, unlike the camelCase locals throughout this client (including
statusRequestinQueryStatus). Rename it and its uses togetOutputRequest.
var GetOutputRequest = CreateGetOutputRequest(request);
policies/rust/now-policy-server-template/assets/samples/responses/output-local-file.response.json:5
- This response identifies
req-winget-vscode-installas its originating request, but that sample request omitsCaptureOutput, which defaults tofalse. Under the new endpoint contract this operation must returnBadRequest, so this successful output fixture contradicts the documented opt-in requirement. Update the originating request fixture to enable capture (and keep the related samples consistent).
"RequestId": "req-winget-vscode-install",
policies/rust/now-policy-server-template/assets/samples/responses/output-http-stream.response.json:5
- This response identifies
req-winget-vscode-installas its originating request, but that sample request omitsCaptureOutput, which defaults tofalse. Under the new endpoint contract this operation must returnBadRequest, so this successful output fixture contradicts the documented opt-in requirement. Update the originating request fixture to enable capture (and keep the related samples consistent).
"RequestId": "req-winget-vscode-install",
Add a per-operation event channel: a one-way local pipe carrying the NOW_BROKER binary frame protocol (Hello, StatusUpdated, Finish, Stdout, Stderr, StdoutOverflow, StderrOverflow). The channel is opened unconditionally when supported and always carries status change notifications; the CaptureOutput flag controls whether stdout/stderr data frames are pushed. Execution responses return an expandable EventChannel descriptor (kind + path). - Implement frame codec in Rust (now-policy-api::event_channel) and C# (Devolutions.Now.Policy.Api EventChannel.cs), validated against a shared binary fixture - Add BrokerClient.OpenEventChannel and OperationEventChannel reader to the .NET client with named-pipe integration tests, including pipe-closure and mid-frame truncation cases - Add protocol specification in policies/docs/event-channel-protocol.md - Regenerate OpenAPI schema Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
8d98474 to
20aa839
Compare
…th limit Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 25 changed files in this pull request and generated no new comments.
Suppressed comments (6)
policies/rust/now-policy-api/src/enums.rs:110
- This closed enum makes the advertised extendable
Kindfield reject any future transport duringEventChanneldeserialization, so even callers that do not use the new transport lose the whole execution response. Use a representation that retains unknown string kinds (and reflect it in the generated schema) so newer brokers remain readable by older consumers.
#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Serialize, Deserialize, JsonSchema, strum::Display)]
#[schemars(rename = "EventChannelKind")]
pub enum EventChannelKind {
/// Local named pipe carrying `NOW_BROKER` event frames.
LocalPipe,
policies/dotnet/Devolutions.Now.Policy.Api/Enums.cs:124
- This string enum cannot represent a future transport kind:
JsonStringEnumConverterthrows on an unknown string while deserializing the entireExecutionResponse. That contradicts the documented extendability ofKindand preventsOpenEventChannelfrom reaching itsUnsupportedCapabilityhandling. Preserve unknown wire values with a forward-compatible converter/value representation.
[JsonConverter(typeof(JsonStringEnumConverter<EventChannelKind>))]
public enum EventChannelKind
{
/// <summary>Local named pipe carrying <c>NOW_BROKER</c> event frames.</summary>
LocalPipe,
policies/dotnet/Devolutions.Now.Policy.Client/OperationEventChannel.cs:84
ReadFramereturns the first decoded frame without enforcing the channel handshake. A stream that starts with data, or aHellowith an unsupported major version, is accepted even though the protocol requiresHellofirst and mandates rejecting unsupported major versions (event-channel-protocol.md:83-91). Track handshake state and reject either condition before exposing frames; add integration tests for both cases.
if (_decoder.TryReadFrame(out var frame))
{
return frame;
policies/dotnet/Devolutions.Now.Policy.Client/BrokerClient.cs:267
- The explanation is inaccurate:
CaptureOutputcontrols only stdout/stderr frames, while the descriptor is present for every operation whenever the broker supports event channels. Reporting missing output capture sends users toward the wrong fix; describe the descriptor as unavailable/unsupported instead.
"The execution response does not carry an event channel descriptor; "
+ "the operation was likely submitted without output capture.");
policies/dotnet/Devolutions.Now.Policy.Client/README.md:19
- This says the descriptor depends on
CaptureOutput, contradicting the new API contract and the sample execution response whose request leaves capture disabled. The broker advertises the channel whenever supported;CaptureOutputonly controls stdout/stderr frames.
Operations submitted with `CaptureOutput` additionally return a per-operation event channel descriptor (`OperationSubmission.EventChannel`) that carries the `NOW_BROKER` frame protocol: stdout/stderr data and status change notifications pushed by the broker. The frame codec (`EventFrame`, `EventFrameDecoder`) lives in `Devolutions.Now.Policy.Api`; see `policies/docs/event-channel-protocol.md` for the wire specification.
policies/docs/event-channel-protocol.md:60
- The reference
Encodemethods reject bodies over 64 KiB; they do not split them, as their tests also assert. Clarify that producers must split output before encoding so the normative protocol text matches both implementations.
- `frame_size` MUST NOT exceed **65536** (64 KiB). Encoders split larger
output into multiple frames; decoders MUST treat a larger value as a fatal
Address review feedback: - OperationEventChannel now enforces the protocol handshake: the first frame must be Hello and its major version must be supported, otherwise EventFrameException is thrown; covered by integration tests - Correct the missing-descriptor error message: descriptor presence depends on broker event channel support, not CaptureOutput - Fix client README wording: the descriptor is returned whenever the broker supports event channels; CaptureOutput only gates stdout/stderr frames - Clarify in the protocol spec that producers split output before encoding; reference Encode implementations reject oversized bodies Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Marc-André Moreau (mamoreau-devolutions)
left a comment
There was a problem hiding this comment.
Looks good to me.
d25e7eb
into
master
Summary
Adds a per-operation event channel for delivering operation output and status-change notifications from the broker to the client, replacing HTTP polling for output entirely.
For each executed operation the broker (when it supports event channels) opens a dedicated channel and returns an expandable
EventChanneldescriptor in the execution response:The channel always carries status-change notifications; the
CaptureOutputrequest flag only controls whether stdout/stderr data frames are pushed over it.The client connects to the pipe and reads a minimal one-way binary frame protocol (
NOW_BROKERframes):u16version major +u16version minorGetStatusHTTP requestu32bytes skippedu32bytes skippedFrame layout is
u32 body_size | u16 kind | body(little-endian, 64 KiB body cap). Decoders ignore unknown frame kinds, keeping the protocol extendable; end-of-stream mid-frame is treated as a truncated-stream error. Full spec:policies/docs/event-channel-protocol.md.Changes
now-policy-api): newevent_channelmodule —EventChanneldescriptor model,EventFrameenum,encode/decode_body, incrementalEventFrameDecoder;OperationSubmission.event_channelfield.Devolutions.Now.Policy.Api):EventChannel.cs— mirrored DTO,EventFramehierarchy,EventFrameDecoder.Devolutions.Now.Policy.Client):BrokerClient.OpenEventChannel(ExecutionResponse)connects to the advertised pipe and returns anOperationEventChannelreader —ReadFrame()for the raw frame stream,ReadEvents()(IAsyncEnumerable) which skips unknown frames and completes afterFinishor EOF.assets/samples/frames/event-channel.frames.bin) validated byte-for-byte by both Rust and .NET test suites, including unknown-frame tolerance; .NET integration tests exercise a real local named pipe, including pipe-closure and mid-frame truncation cases.No crate/package or protocol version bumps (API stays
1.0; pre-release).