Repository navigation
Add Node.js and .NET telemetry SDKs - #822
Conversation
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
Adds stable telemetry consent, policy, and per-run controls to the Node.js and .NET SDKs over the native telemetry contract.
Changes:
- Adds presenter-driven consent and fail-closed policy APIs.
- Propagates telemetry through one-shot and state-aware execution.
- Adds tests, parity/codegen checks, and SDK documentation.
Reviewed changes
Copilot reviewed 33 out of 38 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
tests/scripts/run_telemetry_consent_smoke_test.ps1 |
Normalizes the consent smoke test. |
src/core/wxc_common/src/telemetry/policy.rs |
Normalizes telemetry policy source. |
sdk/node/tests/unit/wire-conformance.test.ts |
Checks telemetry wire conformance. |
sdk/node/tests/unit/telemetry.test.ts |
Tests Node consent APIs. |
sdk/node/tests/unit/state-aware.test.ts |
Tests state-aware telemetry propagation. |
sdk/node/tests/unit/sandbox.test.ts |
Tests one-shot telemetry options. |
sdk/node/src/types.ts |
Promotes telemetry configuration. |
sdk/node/src/telemetry.ts |
Implements Node consent APIs. |
sdk/node/src/state-aware.ts |
Relays state-aware telemetry. |
sdk/node/src/state-aware-types.ts |
Updates telemetry documentation. |
sdk/node/src/state-aware-helper.ts |
Builds top-level telemetry envelopes. |
sdk/node/src/sandbox.ts |
Adds per-invocation telemetry options. |
sdk/node/src/index.ts |
Exports telemetry APIs. |
sdk/node/src/helper.ts |
Applies one-shot telemetry overrides. |
sdk/node/src/generated/wire.ts |
Updates generated stable wire types. |
sdk/node/src/generated/telemetry-consent-wire.ts |
Adds generated consent wire types. |
sdk/node/README.md |
Documents Node telemetry usage. |
sdk/node/package.json |
Runs telemetry unit tests. |
sdk/dotnet/README.md |
Documents .NET telemetry usage. |
sdk/dotnet/Microsoft.Mxc.Sdk/TelemetryPolicyState.cs |
Defines policy states. |
sdk/dotnet/Microsoft.Mxc.Sdk/TelemetryConsentState.cs |
Defines consent states. |
sdk/dotnet/Microsoft.Mxc.Sdk/TelemetryConsent.cs |
Defines consent models. |
sdk/dotnet/Microsoft.Mxc.Sdk/StateAwareTypes.cs |
Adds lifecycle telemetry options. |
sdk/dotnet/Microsoft.Mxc.Sdk/SandboxPolicy.cs |
Adds one-shot telemetry opt-in. |
sdk/dotnet/Microsoft.Mxc.Sdk/Native/NativeLibraryResolver.cs |
Improves native profile resolution. |
sdk/dotnet/Microsoft.Mxc.Sdk/MxcTelemetry.cs |
Implements .NET consent APIs. |
sdk/dotnet/Microsoft.Mxc.Sdk/MxcLifecycle.cs |
Propagates lifecycle telemetry. |
sdk/dotnet/Microsoft.Mxc.Sdk/MxcException.cs |
Preserves underlying exceptions. |
sdk/dotnet/Microsoft.Mxc.Sdk/Microsoft.Mxc.Sdk.csproj |
Builds profile-specific native libraries. |
sdk/dotnet/Microsoft.Mxc.Sdk/ErrorCode.cs |
Adds consent-write failure code. |
sdk/dotnet/Microsoft.Mxc.Sdk.Tests/MxcTelemetryTests.cs |
Tests .NET consent and policy behavior. |
sdk/dotnet/Microsoft.Mxc.Sdk.Tests/MxcLifecycleTests.cs |
Tests lifecycle telemetry envelopes. |
scripts/versioning/check-telemetry-consent-codegen.js |
Verifies generated consent artifacts. |
scripts/check-telemetry-policy-parity.js |
Checks cross-language policy states. |
README.md |
Documents stable telemetry behavior. |
docs/telemetry/telemetry-policy.md |
Normalizes administrative policy docs. |
.github/workflows/Versioning.Checks.Job.yml |
Adds telemetry parity checks. |
.github/copilot-instructions.md |
Records telemetry architecture and commands. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 33 out of 38 changed files in this pull request and generated no new comments.
Suppressed comments (6)
sdk/dotnet/README.md:133
- The referenced
telemetry-administrative-policy.mdfile does not exist; the policy guide added to the repository isdocs/telemetry/telemetry-policy.md. Point this link at the actual document.
`Allowed` does not grant user consent, while `Blocked` disables collection and
the consent prompt. The policy query fails closed to `Blocked` if the native
library cannot be loaded; non-Windows hosts return `NotApplicable`. See
[`docs/telemetry/telemetry-administrative-policy.md`](../../docs/telemetry/telemetry-administrative-policy.md).
sdk/node/README.md:516
- This link targets
telemetry-administrative-policy.md, which does not exist; the administrative policy document in this PR isdocs/telemetry/telemetry-policy.md. Update the URL so published package documentation does not lead to a 404.
`'allowed'` does not grant user consent, while `'blocked'` disables collection
and the consent prompt. An unreadable or missing `policy` field reads back as
`'blocked'`; non-Windows hosts return `'not-applicable'`. See
[`docs/telemetry/telemetry-administrative-policy.md`](https://github.com/microsoft/mxc/blob/main/docs/telemetry/telemetry-administrative-policy.md).
sdk/dotnet/Microsoft.Mxc.Sdk/TelemetryPolicyState.cs:8
- This XML documentation references a nonexistent
telemetry-administrative-policy.md; the repository's administrator-facing guide isdocs/telemetry/telemetry-policy.md. Correct the path so generated API docs direct users to a real file.
/// <summary>
/// The administrative (MDM / Group Policy) telemetry decision for this machine.
/// See docs/telemetry/telemetry-administrative-policy.md for the admin-facing reference.
sdk/node/src/telemetry.ts:153
- A presenter may legally
throw undefinedin JavaScript. In that case this assignment leavespresenterFailureequal to the “no failure” sentinel, so the close handler can resolve the dismissed native response instead of propagating the presenter failure. Track failure with a separate boolean or normalize every caught value to anError.
sdk/node/src/telemetry.ts:353 - This convenience getter discards
queryTelemetryConsent().error, so a missing/mismatched executable is indistinguishable from a genuine undecided user. Consent reads are required to surface failures through the binding's documented exception type; only prompt/policy queries should silently fail closed. Check the query result and throw anMxcErrorwhenerroris present.
sdk/dotnet/Microsoft.Mxc.Sdk/MxcTelemetry.cs:65 - Returning
Undeterminedfor native-load failures makes a broken or outdated native installation indistinguishable from a user who has not decided. Consent reads must raise the documentedMxcExceptionon infrastructure failures; reserve silent fail-closed behavior forNeedsConsentPromptandGetPolicy. Wrap this failure asMxcExceptioninstead of returning a consent state.
catch (Exception ex) when (IsNativeLoadFailure(ex))
{
ReportFailClosed("GetConsent", "Undetermined", ex);
return TelemetryConsentState.Undetermined;
}
c8415af to
2f8f372
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 54 out of 58 changed files in this pull request and generated 1 comment.
Suppressed comments (6)
src/core/wxc_common/src/telemetry/events.rs:119
- This always derives the error event's
sandbox_kindfromctx.backend, so callers that resolved a broad SDK request such asprocessorvmemit paired records with different attribution:MXC.Executionkeeps the requested kind, whileMXC.Errorreports the concrete backend. Pass the request-scoped sandbox kind through the error path as well so both events remain joinable and semantically consistent.
sdk/dotnet/Microsoft.Mxc.Sdk/SandboxPolicy.cs:41 - The FFI contract identifies nested
telemetry.enabledas canonical andtelemetryEnabledonly as a legacy compatibility alias (src/ffi/mxc_ffi/src/lib.rs:31-35). Serializing the newly shipped .NET surface through that alias makes new clients depend on the compatibility path; retain the convenience property but project it onto the canonical nested section.
[JsonPropertyName("telemetryEnabled")]
public bool? TelemetryEnabled { get; set; }
sdk/node/src/telemetry.ts:376
- This catch handles every transport/parser failure, including a missing executable, timeout, and malformed child output, but labels all of them as
store-unreadable. That reason specifically describes consent-store state and is false for these failures; omit it here and let the diagnosticerrordistinguish transport failures.
src/mxc_telemetry/src/lib.rs:358 - This source-text assertion is self-fulfilling:
provider_sourceincludes this test, so the searchedstr8("mxc.sandbox_kind", sandbox_kind)literal still appears later in the file even if the productionMXC.Errorfield is removed. Bound the extracted event body before the test module or assert captured/decoded event fields instead.
.github/copilot-instructions.md:228 - This changes the reference to
docs/telemetry/telemetry-policy.md, but that file does not exist; the checked-in policy document isdocs/telemetry/telemetry-administrative-policy.md. Keep the existing filename unless this PR also renames the document and updates all remaining SDK links.
- `docs/telemetry/telemetry.md` — telemetry overview; `docs/telemetry/telemetry-consent-design.md` (Windows-only consent design and per-SDK surface) and `docs/telemetry/telemetry-policy.md` (the MDM / Group Policy ceiling)
.github/copilot-instructions.md:333
- The referenced
docs/telemetry/telemetry-policy.mdis absent; the repository currently containstelemetry-administrative-policy.md, which the Node.js and .NET README links also use. Point this convention at the existing document or include a coordinated rename.
- **Telemetry consent or policy changes** → update `docs/telemetry/telemetry-consent-design.md` and/or `docs/telemetry/telemetry-policy.md`, and keep `scripts/check-telemetry-policy-parity.js` green across all three bindings
There was a problem hiding this comment.
🟡 Changes recommended
A critical test compilation failure and a moderate parity-gate contract mismatch remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 36/36 changed files
- Comments generated: 2
- Review effort level: Balanced
There was a problem hiding this comment.
🔵 Needs a closer look
The parity gate must validate .NET against the Rust SDK/C ABI contract rather than the differing CLI protocol.
Review details
Suppressed comments (1)
scripts/check-telemetry-policy-parity.js:15
- This gate validates the .NET binding against the executor CLI protocol, but .NET consumes the Rust SDK/C ABI contract. Those producers already differ: the CLI adds
presentationUnavailableand two CLI-only reasons, whilemxc-sdk/src/telemetry.rsdoes not expose them. As written, CI can pass even when the actual C ABI and C# binding drift. Derive the Node expectations fromconsent_protocol.rsand the C# expectations from the Rust SDK/C ABI producer separately.
const policy = readFileSync(
join(root, "src", "core", "wxc_common", "src", "telemetry", "policy.rs"),
"utf8"
- Files reviewed: 36/36 changed files
- Comments generated: 0 new
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
Node.js consent-response validation must require reason so malformed responses fail closed.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 36/36 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
🔵 Needs a closer look
A moderate fail-closed consent-protocol flaw remains, and the cross-language native-boundary scope warrants human review.
Review details
Suppressed comments (1)
sdk/node/src/telemetry.ts:329
- Reject protocol lines that arrive before the presenter response is written. The native transport writes
presentationRequiredand then blocks inread_response(consent_cli.rs:422-426), so a terminal line received while the presenter is still pending is necessarily a protocol violation. Because this queue waits for the presenter and then processes any already-buffered terminal line, a malformed/stale executable can reportgrantedindependently of the user's returned decision; the test atdefault-consent-protocol-runner.test.ts:315-332currently permits exactly that sequence. Track the presentation-response boundary at receive time and fail closed on premature output.
lineQueue = lineQueue
.then(() => processLine(line))
.catch(fail);
- Files reviewed: 36/36 changed files
- Comments generated: 0 new
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
The mutable consent prompt can produce an invalid protocol reply and reject an otherwise valid decision.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 36/36 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
🔵 Needs a closer look
The Node.js consent protocol can write a presenter decision after its deadline has already rejected the request.
Review details
Suppressed comments (1)
sdk/node/src/telemetry.ts:296
- If the remaining protocol deadline expires while the presenter is running,
resumeProtocolDeadline()callsfail()and marks the request rejected, but execution still continues and writes the presenter's decision to the child. Near the deadline this can let a consent decision reach the native process after the SDK has reported a timeout. Return immediately when resuming settles the request so a failed request cannot persist consent.
resumeProtocolDeadline();
- Files reviewed: 36/36 changed files
- Comments generated: 0 new
- Review effort level: Balanced
|
lateProc is defined inline as the lambda parameter here:
lateProc => lateProc.Dispose()
It's not a separate variable or member. Type inference makes it an MxcSandboxProcess because ExecInSandbox(...) returns that type and RunBlockingOperationAsync<T> accepts an Action<T> ( MxcLifecycle.cs:177 , 310-345 ).
It represents a process handle returned after the caller already canceled; the callback disposes that late result to prevent a resource leak. The name could be clearer as:
(MxcSandboxProcess process) => process.Dispose()
|
There was a problem hiding this comment.
🟡 Changes recommended
Moderate cancellation handling and unbounded Node.js failure-tracking issues remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 37/37 changed files
- Comments generated: 2
- Review effort level: Balanced
There was a problem hiding this comment.
🔵 Needs a closer look
Two moderate issues remain in C# parity validation and Node.js diagnostic deduplication.
Review details
Suppressed comments (2)
scripts/check-telemetry-policy-parity.js:170
- The C# SDK binds the FFI contract, but this derives its expected results/reasons from the executor-only consent protocol. That protocol adds
presentationUnavailableandpolicy-blocked/presentation-unavailable, whereasmxc-sdk::telemetryandconsent_outcome_jsoncannot emit them. Consequently the parity check enforces extra C# values rather than verifying the managed binding against its native source. Derive separate TypeScript CLI and C# FFI sets from their actual Rust contracts.
const terminalResults = rustEnum("ConsentResult");
terminalResults.delete("status");
terminalResults.delete("presentationRequired");
compareSets(
"TypeScript consent result",
terminalResults,
typescriptValues("TELEMETRY_CONSENT_RESULTS")
sdk/node/src/telemetry.ts:581
- This deduplication set is process-global and unbounded, while its key includes the full failure detail. A long-running host that repeatedly receives distinct native diagnostics or malformed outputs will retain every string for the process lifetime. Please cap the tracker (the .NET surface uses 64 categories) while preserving once-per-category reporting.
const reportedFailureCategories = new Set<string>();
- Files reviewed: 36/36 changed files
- Comments generated: 0 new
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
State-aware telemetry now depends on an emitted schema-version constant that is not protected by the canonical version parity gate.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 36/36 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
The Node consent runner can write a decision after its protocol deadline has already failed.
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
There was a problem hiding this comment.
🟡 Changes recommended
Node.js must fail closed when native responses contain inconsistent needsPrompt values.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
sdk/node/src/telemetry.ts:653
- Also constrain the query result to the native prompting invariant. As written,
needsPrompt: trueis exposed forgranted,denied, ornot-applicableeffective states whenever policy is not blocked, so a semantically corrupt response can trigger an inappropriate consent prompt rather than fail closed.
};
- Files reviewed: 37/37 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
Public result types and the parity gate include a presenter-failure outcome that the production SDK contracts cannot return.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
scripts/check-telemetry-policy-parity.js:166
- This derives the .NET result set from the executor's private CLI protocol, not from the FFI contract that .NET consumes.
mxc_sdk::telemetry::ConsentActionResultomitspresentationUnavailable, andmxc_telemetry_request_consentmaps presenter failures toBackendError; therefore this gate forces an unreachable .NET enum member and does not actually check C#/FFI parity. Split CLI/TypeScript parity from FFI/C# parity and derive the latter from the Rust SDK result enum.
const terminalResults = rustEnum("ConsentResult");
terminalResults.delete("status");
terminalResults.delete("presentationRequired");
- Files reviewed: 37/37 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
The telemetry parity job currently fails because Node.js omits the native presentationUnavailable terminal result.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 37/37 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
🔵 Needs a closer look
The legacy telemetry-field rejection bypasses the Node.js SDK’s typed error contract.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
sdk/node/src/helper.ts:215
- This SDK-side schema rejection throws a plain
Error, so callers lose the typedMxcError.codethey would receive from native request validation. Use the already importedmxcErrorFromCodeand classify this asmalformed_requestso programmatic error handling remains intact.
- Files reviewed: 37/37 changed files
- Comments generated: 0 new
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
The Node one-shot validator can throw an unintended raw TypeError for malformed experimental values.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 37/37 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
🔵 Needs a closer look
A newly added lifecycle test contains an ambiguous overload call that prevents the .NET test project from compiling.
Review details
Suppressed comments (1)
sdk/dotnet/Microsoft.Mxc.Sdk.Tests/MxcLifecycleTests.cs:678
- This call is ambiguous between the three-parameter overload whose third argument is
CancellationTokenand the four-parameter overload whose third argument isStateAwareExecOptions?; adefaultliteral converts to both, so the test project will fail with CS0121. Name the argument (or explicitly type it) to select the cancellation-token overload.
MxcLifecycle.ExecInSandboxAsync(id, command, default);
- Files reviewed: 37/37 changed files
- Comments generated: 0 new
- Review effort level: Balanced
There was a problem hiding this comment.
🔵 Needs a closer look
The privacy-sensitive cross-language consent protocol and native integration require final human validation despite extensive automated coverage.
Review details
- Files reviewed: 37/37 changed files
- Comments generated: 0 new
- Review effort level: Balanced
There was a problem hiding this comment.
🔵 Needs a closer look
The privacy-sensitive cross-language consent protocol and native interop breadth warrant final human validation despite no confirmed defects.
Review details
- Files reviewed: 59/60 changed files
- Comments generated: 0 new
- Review effort level: Balanced
| /// # Safety | ||
| /// `out_utf8` must point to writable pointer-sized storage. | ||
| #[no_mangle] | ||
| pub unsafe extern "C" fn mxc_telemetry_withdraw_consent(out_utf8: *mut *mut c_char) -> i32 { |
Add fail-closed telemetry consent APIs for Node.js and refine the .NET consent surface. Propagate telemetry through one-shot and state-aware envelopes, enforce cross-language vocabulary parity, and cover cancellation, malformed responses, release-safe behavior, and consent-result parity. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6e20d046-910e-4ee7-b8e8-acb0fe007208
.github/copilot-instructions.md.Summary
Adds the Node.js and .NET telemetry surfaces over the reviewed native contract: presenter-driven consent, policy-aware status, withdrawal, per-run options for one-shot and state-aware execution, generated wire types, parity checks, tests, and final SDK/product documentation.
This is PR 5 of 5 and depends on the Rust SDK/C ABI in PR 4.
Stack
Review only this PR's diff; prerequisite behavior is in the PR above.