Repository navigation
Expose telemetry through Rust SDK, C ABI, and .NET binding - #821
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
1f67a37 to
0ca5177
Compare
Co-authored-by: RamonArjona4 <25335379+RamonArjona4@users.noreply.github.com>
Co-authored-by: RamonArjona4 <25335379+RamonArjona4@users.noreply.github.com>
…ust-sdk-ffi' into user/ramonarjona4/telemetry-04-rust-sdk-ffi Co-authored-by: RamonArjona4 <25335379+RamonArjona4@users.noreply.github.com>
Co-authored-by: RamonArjona4 <25335379+RamonArjona4@users.noreply.github.com>
Document consent cancellation as best-effort when native persistence wins, and restore strict canonical SandboxRequest deserialization with round-trip and malformed-input coverage. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6e20d046-910e-4ee7-b8e8-acb0fe007208
…duplicate properties Co-authored-by: RamonArjona4 <25335379+RamonArjona4@users.noreply.github.com>
Document consent cancellation as best-effort when native persistence wins, and restore strict canonical SandboxRequest deserialization with round-trip, malformed-input, duplicate-property, and platform-boundary coverage. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6e20d046-910e-4ee7-b8e8-acb0fe007208
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6e20d046-910e-4ee7-b8e8-acb0fe007208
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6e20d046-910e-4ee7-b8e8-acb0fe007208
Co-authored-by: RamonArjona4 <25335379+RamonArjona4@users.noreply.github.com>
Document why the managed authoring model needs an explicit canonical wire projection while native request validation remains authoritative. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6e20d046-910e-4ee7-b8e8-acb0fe007208
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
Canonical .NET deserialization can accept invalid input and silently alter network enforcement.
Review details
Suppressed comments (3)
Previously missed (1) — in code that hasn't changed since the last review.
sdk/dotnet/Microsoft.Mxc.Sdk/CanonicalRequestBuilder.cs:26
- The canonical reader currently accepts numeric enum tokens because
JsonStringEnumConverterallows integers by default. That lets inputs such as"clipboard": 1pass managed deserialization and be normalized into valid string JSON before the native parser can reject the noncanonical input. Disable integer enum values so this bridge preserves the canonical wire boundary.
sdk/dotnet/Microsoft.Mxc.Sdk/CanonicalRequestBuilder.cs:352
enforcementModeis validated here but then discarded. Re-serializing the resulting request recomputes the mode from the current host/containment, so a canonicalfirewall,both, orcapabilitiesrequest can silently change network enforcement. Preserve the mode in the managed projection, or reject it as unrepresentable instead of accepting and dropping it.
if (network.TryGetProperty("enforcementMode", out var enforcementMode))
{
_ = ReadString(enforcementMode, "network.enforcementMode") switch
{
"capabilities" or "firewall" or "both" => true,
sdk/dotnet/Microsoft.Mxc.Sdk/CanonicalRequestBuilder.cs:539
- Typed nested sections deserialized through this helper do not pass through
EnsureKnownProperties, so duplicate keys inside structures such asnetwork.egressare accepted with last-value-wins semantics. The native canonical parser rejects duplicates; managed deserialization therefore normalizes an otherwise rejected policy before execution. Recursively reject duplicate object properties before typed deserialization.
private static T DeserializeElement<T>(JsonElement element, string location)
{
try
{
return element.Deserialize<T>(CanonicalReadOptions)
?? throw new JsonException($"{location} cannot be null.");
- Files reviewed: 37/38 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Return one-shot language bindings to the co-versioned RequestSpec envelope and parse telemetry at the FFI adapter before constructing SandboxRequest. Remove the managed canonical request converter and hidden engine bridge while preserving strict malformed-input validation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6e20d046-910e-4ee7-b8e8-acb0fe007208
There was a problem hiding this comment.
🔵 Needs a closer look
The privacy-sensitive, cross-language ABI change requires final human validation across native and managed runtime environments.
Review details
- Files reviewed: 30/31 changed files
- Comments generated: 0 new
- Review effort level: Balanced
# Conflicts: # sdk/dotnet/README.md
There was a problem hiding this comment.
🟡 Changes recommended
Malformed null consent-prompt fields can be silently rendered as empty strings before a grant is persisted.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 30/31 changed files
- Comments generated: 1
- Review effort level: Balanced
|
Following up on the telemetry placement question in #821 (comment): telemetry is intentionally not a field on the engine |
Keep schema-version validation with the ProcessContainer authoring code and remove the duplicate shared-parser check. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6e20d046-910e-4ee7-b8e8-acb0fe007208
There was a problem hiding this comment.
🔵 Needs a closer look
Public Rust, C, and .NET documentation incorrectly describes consent alone as effective telemetry authorization despite the separate administrative-policy gate.
Review details
Suppressed comments (4)
Previously missed (4) — in code that hasn't changed since the last review.
sdk/dotnet/Microsoft.Mxc.Sdk/MxcTelemetry.cs:277
GetConsent()does not include the administrative policy gate, soGrantedcan be returned whileGetPolicy()isBlocked. Calling this state “effective for telemetry authorization” may lead consumers to bypass the required policy check; document it as the validated consent component and direct callers to check policy separately.
src/core/mxc-sdk/src/telemetry.rs:22Grantedonly reflects the validated MXC consent record; an administrative block or an invocation without telemetry enabled still keeps telemetry off. This example currently teaches callers to equate consent with final authorization, so label it as consent state (or explicitly combine the policy and per-run gates).
src/core/mxc-sdk/src/telemetry.rs:347- This description overstates what
get_consent()computes. The delegated consent status validates the persisted record and prompt version but does not evaluate the separate administrative policy, so it can returnGrantedwhileget_policy()isBlocked. Clarify that callers must evaluate policy as well before treating collection as authorized.
src/ffi/mxc_ffi/src/lib.rs:542 - This export only returns the validated consent record; it does not apply the administrative policy, so it may return
"granted"whilemxc_telemetry_get_policyreturns"blocked". Calling it effective authorization is misleading for C consumers; explicitly require the policy check as well.
- Files reviewed: 29/30 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Remove the stale parser-level validation test after restoring ProcessContainer ownership, and reject null required consent prompt strings before invoking a presenter. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6e20d046-910e-4ee7-b8e8-acb0fe007208
There was a problem hiding this comment.
🟡 Changes recommended
The streaming integration-test helper emits the wrong request shape, causing all real streaming tests to fail before process launch.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 28/29 changed files
- Comments generated: 1
- Review effort level: Balanced
Co-authored-by: RamonArjona4 <25335379+RamonArjona4@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
The parity gate loses direct coverage of managed discriminators and the FFI request adapter, and the .NET telemetry version requirement is undocumented.
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.
sdk/dotnet/Microsoft.Mxc.Sdk/SandboxPolicy.cs:65
- The native binding rejects
policy.telemetryfor schema versions before0.9.0-alpha, but this new public property does not disclose that requirement. Existing callers commonly use the documented0.7.0-alphaexample, so addingTelemetryproduces aMalformedRequest; document the minimum version here so the failure is predictable.
- Files reviewed: 28/29 changed files
- Comments generated: 1
- Review effort level: Balanced
| containment: &Containment, | ||
| container_name: Option<&str>, | ||
| ) -> Result<(serde_json::Value, NetworkFormat), MxcError> { | ||
| use serde_json::json; |
There was a problem hiding this comment.
i don't really understand this subtle change, but good enough.
.github/copilot-instructions.md.Summary
Exposes stable telemetry consent and policy through the public Rust SDK, panic-safe C ABI, and managed .NET binding. Adds typed presenter/status/withdrawal APIs, native binding entry points, streaming and state-aware propagation, and binding-codegen coverage.
This is PR 4 of 5 and depends on the stable runtime integration in PR 3.
Stack
Review only this PR's diff; prerequisite behavior is in the PR above.