Add telemetry consent and policy foundation - #819
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 Windows telemetry consent, administrative policy enforcement, localized consent resources, and balanced ETW provider lifetime management.
Changes:
- Adds persisted, versioned consent and withdrawal APIs.
- Adds deny-only machine policy handling.
- Embeds canonical consent resources and reference-counts ETW registration.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/mxc_telemetry/src/lib.rs |
Reference-counts ETW registration. |
src/core/wxc_common/src/telemetry/policy.rs |
Implements administrative policy lookup. |
src/core/wxc_common/src/telemetry/mod.rs |
Exposes consent and policy modules. |
src/core/wxc_common/src/telemetry/consent.rs |
Implements consent persistence and actions. |
src/core/wxc_common/src/telemetry/consent_prompt.rs |
Exposes canonical prompt resources. |
src/core/wxc_common/resources/telemetry/consent/en-US.json |
Defines English consent wording. |
src/core/wxc_common/Cargo.toml |
Adds build and test-support configuration. |
src/core/wxc_common/build.rs |
Validates and embeds consent resources. |
docs/telemetry/telemetry-consent-design.md |
Updates consent design documentation. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if *registered > 0 { | ||
| // Already registered — the tracelogging crate panics on double | ||
| // register, so we must not call `register()` again. | ||
| // register, so retain a reference instead. | ||
| *registered += 1; |
| /// administrator rights. The override is compiled out of every shipped | ||
| /// binary, so a release build can never be pointed at a user-writable key. | ||
| fn policy_location() -> (winreg::HKEY, String) { | ||
| #[cfg(any(test, debug_assertions))] | ||
| if let Some(subkey) = debug_policy_key_override() { | ||
| return (winreg::enums::HKEY_CURRENT_USER, subkey); | ||
| } | ||
| (HKEY_LOCAL_MACHINE, POLICY_SUBKEY.to_string()) | ||
| } | ||
|
|
||
| /// Test-only hook: redirects the policy read to `HKCU\<value>`. | ||
| /// | ||
| /// Active under `cfg(test)` (this crate's own harness — CI runs | ||
| /// `cargo test --release`, so a `debug_assertions`-only gate would drop | ||
| /// every policy test from CI and make them read real machine policy) or | ||
| /// in a debug build (for `mxc_ffi`'s cross-crate tests). Never present in | ||
| /// a binary MXC ships, which is neither. | ||
| #[cfg(any(test, debug_assertions))] | ||
| fn debug_policy_key_override() -> Option<String> { |
| /// Persists a new telemetry consent decision for the current Windows user. | ||
| /// | ||
| /// `source` is free-form provenance (e.g. `"prompt"`, `"settings-toggle"`, | ||
| /// `"cli"`) recorded alongside the decision for support/debugging; it is | ||
| /// never transmitted anywhere and never affects gating. | ||
| /// | ||
| /// Returns an error string suitable for CLI/log output. On non-Windows this | ||
| /// always fails with a descriptive "not applicable" error — MXC must not | ||
| /// silently accept a consent decision it can never act on. | ||
| pub fn set_consent(granted: bool, source: &str) -> Result<(), String> { | ||
| platform::write(granted, source) | ||
| } |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Suppressed comments (4)
src/core/wxc_common/src/telemetry/consent.rs:682
- A current-version grant is accepted for any nonempty
promptLocale, including values this build could never have presented (for example"fr-FR"or arbitrary text). Since the only embedded prompt currently resolves toen-US, such a malformed record must fail closed rather than authorize collection; validate the locale against the embedded supported resources.
Some(CONSENT_RESOURCE_VERSION) if !record.prompt_locale.is_empty() => ConsentStatus {
src/core/wxc_common/src/telemetry/consent.rs:230
- The public API documentation says an unrecognized schema always resolves to
Undetermined, but a record containing"consent":"denied"resolves toDeniedregardless of schema version. Document the preserved-denial behavior so callers do not rely on an incorrect status contract.
/// Returns the current, persisted telemetry consent state.
///
/// Fail-closed: a missing file, an unreadable file, unparseable JSON, or an
/// unrecognized `schemaVersion` all resolve to [`ConsentState::Undetermined`]
/// — never to `Granted`. Always [`ConsentState::NotApplicable`] on
/// non-Windows platforms, without any filesystem access.
src/core/wxc_common/src/telemetry/consent.rs:679
- An unknown schema such as
999is reported asprompt-version-missingwhenever its prompt field is absent, even thoughConsentSchemaUnsupportedis the reason defined for this case. Reserveprompt-version-missingfor the known schema-1 legacy record so status consumers receive an accurate diagnosis for unknown formats.
This issue also appears on line 682 of the same file.
reason: Some(if record.prompt_resource_version.is_none() {
ConsentStatusReason::PromptVersionMissing
} else {
ConsentStatusReason::ConsentSchemaUnsupported
}),
src/core/wxc_common/src/telemetry/consent.rs:32
- This states that every older or unknown schema becomes
Undetermined, butread_statusintentionally preserves a stored denial before checking the schema version. Narrow the wording to grants so the persistence contract matches the implementation and the design document's “legacy denial remains denied” rule.
This issue also appears on line 225 of the same file.
/// Current schema version for the persisted consent record. Bump when the
/// on-disk shape changes in a way that isn't purely additive; unknown/older
/// versions are treated as [`ConsentState::Undetermined`] on read (fail
/// closed) rather than guessed at.
| ) -> Result<ConsentActionOutcome, ConsentActionError> { | ||
| match decision { | ||
| ConsentDecision::Yes => { | ||
| platform::write_presented(true, "prompt", prompt) |
There was a problem hiding this comment.
Medium (security/reliability) — Re-check policy and consent state before persisting the presenter's decision.
consent_preflight snapshots policy/status before invoking a presenter that may remain open indefinitely, and this write uses that stale snapshot. If policy becomes blocked while the UI is open, the code still persists a dormant grant and returns the old policy in ConsentActionOutcome; an intervening decision from another surface can likewise be overwritten by the stale dialog.
Fix: Re-read policy and status immediately before an affirmative write. Return PolicyBlocked or a typed state-changed result when the preflight conditions no longer hold.
| reason: Some(ConsentStatusReason::StoreUnreadable), | ||
| }; | ||
| }; | ||
| let data = match with_io_retry(|| fs::read_to_string(&path)) { |
There was a problem hiding this comment.
Medium (reliability) — Bound the consent-file read.
The consent file lives in a user-writable directory, but read_to_string allocates for the entire file before parsing the tiny record. A malformed or hostile same-user process can replace it with an arbitrarily large file and force excessive allocation whenever consent is queried.
Fix: Read through a small fixed upper bound comfortably above the schema size, and fail closed with StoreMalformed/StoreUnreadable when the file exceeds it.
| let unique = format!("{}-{:x}", std::process::id(), random_suffix()); | ||
| let tmp_path = path.with_extension(format!("json.{unique}.tmp")); | ||
|
|
||
| if let Err(e) = with_io_retry(|| fs::write(&tmp_path, &json)) { |
There was a problem hiding this comment.
Medium (reliability) — Flush the new record before replacing the old decision.
The temp file is written and immediately renamed without sync_data/sync_all. Rename prevents ordinary torn writes, but it does not make the new bytes durable across a crash or power loss. Losing a withdrawal can leave the prior granted record as the effective decision after restart.
Fix: Write through an explicit file handle, flush and sync it, then atomically replace the destination; sync the containing directory where the platform supports it.
| } | ||
|
|
||
| /// Asynchronous counterpart to [`request_consent`]. | ||
| pub async fn request_consent_async<F, Fut>( |
There was a problem hiding this comment.
Medium (performance) — The async API performs blocking storage work on the caller's executor thread.
This function calls synchronous registry/file preflight before its only await, then performs synchronous persistence and another status read afterward. Those paths can also execute thread::sleep retries. Awaiting this from a single-threaded or latency-sensitive runtime therefore blocks unrelated tasks.
Fix: Offload preflight and persistence to a blocking executor, or make the storage operations genuinely asynchronous. If runtime neutrality prevents that here, document the blocking contract explicitly rather than presenting the function as non-blocking.
| /// provider's real state out of sync (e.g. flag set to registered after a | ||
| /// `register()` that actually failed, or a double register/unregister). | ||
| static REGISTERED: Mutex<bool> = Mutex::new(false); | ||
| static REGISTERED: Mutex<usize> = Mutex::new(0); |
There was a problem hiding this comment.
Low (test coverage) — Stress concurrent emission against final provider shutdown.
The new mutex/refcount tests verify nested init/shutdown, but every provider test is serialized and no test logs while another thread releases the final registration. That is the concurrency boundary this refcount now owns.
Fix: Add a bounded multithreaded test that emits while another thread repeatedly acquires/releases registrations, asserting no panic, deadlock, or inconsistent final is_active state.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Suppressed comments (4)
src/core/wxc_common/src/telemetry/mod.rs:300
- This has the same claim-versus-completion race as the completion path: a panic or console handler can claim
HAS_EMITTEDand still be writing when this branch unregisters the provider. Returning without shutdown on the losing path avoids truncating the winning terminal emission; process teardown cleans up when the out-of-band path wins.
shutdown();
src/core/wxc_common/src/telemetry/mod.rs:670
- A state-aware terminal path can also race the installed panic/control handler. Since
already_emitted()indicates only that another path claimed the slot—not that its writes finished—unregistering here can cut off that winner's ETW writes. Leave cleanup to the winner or process teardown when this path loses.
shutdown();
src/core/wxc_common/src/telemetry/consent.rs:1258
- This 40 ms wall-clock bound can fail solely because the test process was descheduled, making the Windows test suite flaky. The preceding injected-operation test deterministically verifies that
NotFoundperforms one attempt; remove this timing-only test or replace elapsed-time measurement with an injected sleeper/counter.
assert!(
started.elapsed() < std::time::Duration::from_millis(40),
"fresh-store read took {:?}; the retry loop is sleeping on NotFound again",
started.elapsed()
);
src/core/wxc_common/src/telemetry/consent.rs:1243
- This wall-clock assertion is nondeterministic: scheduler preemption or a slow CI host can exceed 20 ms even though the closure was called only once and no retry sleep occurred. The call-count assertion already proves
NotFoundwas not retried, so keep that deterministic check and remove the timing bound.
This issue also appears on line 1254 of the same file.
assert!(
started.elapsed() < std::time::Duration::from_millis(20),
"NotFound must not sleep on the retry delay"
);
| }); | ||
| } | ||
|
|
||
| platform::write(false, "withdrawal").map_err(ConsentActionError::Persist)?; |
469850f to
48383e0
Compare
microsoft-github-policy-service
Bot
added
the
Copilot-Instructions
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated 2 comments.
Suppressed comments (7)
src/core/wxc_common/src/telemetry/mod.rs:185
- These consent and policy checks run only during provider initialization; all completion/crash paths later trust the cached
activeflag. If consent is withdrawn orAllowTelemetrybecomes blocking while a sandbox runs, the process still writes its terminal events. The prerequisite contract requires both states to be checked immediately before every event write. Re-check them centrally inlog_executionandlog_error, while still shutting down the provider when an event is suppressed.
config.enabled.unwrap_or(false)
&& consent::get_consent().allows_collection()
&& policy::get_policy().allows_collection()
sdk/node/src/state-aware.ts:36
- This process-local map conflicts with the state-aware contract that the persisted
SandboxIdis the only handle and MXC retains no state between calls. After a consumer restart, the sandbox ID still resumes the lifecycle but this map is empty; subsequent envelopes omit the vector and the executor reseeds, so the lifecycle's telemetry can no longer be correlated. Keep the correlation in backend-owned durable lifecycle metadata or derive it from a durable opaque handle instead of module memory.
const lifecycleCorrelationBySandboxId = new Map<string, string>();
function correlationForSandbox(sandboxId: string): string | undefined {
return lifecycleCorrelationBySandboxId.get(sandboxId);
.github/copilot-instructions.md:152
tests/scripts/run_telemetry_consent_smoke_test.ps1does not exist in this repository or in the PR, so this documented command cannot be run. Remove the entry until that stacked change lands, or include the script here.
tests\scripts\run_telemetry_consent_smoke_test.ps1 # Telemetry consent + policy CLI E2E (Windows; debug binary only)
tests/scripts/run_telemetry_etw_smoke_test.ps1:190
- This drops
mxc.sandbox_kindfrom the only payload-level ETW smoke validation even though this PR adds it as a required public field. The source-text unit test does not verify the actual TraceLogging XML payload, so keep this field in the expected list.
$expectedFields = @('mxc.backend', 'mxc.exit_code', 'mxc.outcome', 'mxc.duration_ms')
docs/schema.md:141
docs/telemetry/telemetry.mdhas nocorrelating-a-lifecycleheading, so this newly added fragment points to a nonexistent section. Link to the existing document or add the referenced section.
> and [`docs/telemetry/telemetry.md`](telemetry/telemetry.md#correlating-a-lifecycle).
sdk/dotnet/README.md:115
- No .NET consent or policy query surface is added in this PR; the stack explicitly assigns those SDK bindings to PR #822. This present-tense statement therefore documents APIs that current consumers cannot call. Retain the forward-looking wording until the query APIs land.
for the stable registry contract and interaction rules. Policy and consent
queries fail closed rather than upgrading an unreadable device state into
collection.
src/core/wxc_common/src/telemetry/policy.rs:188
- This registry failure is silently collapsed into
Blocked. The newly added telemetry convention in.github/copilot-instructions.md:318requires every swallowed fail-closed error to be reported once per distinct failure, because callers cannot distinguish this from a legitimate policy block. Add the shared non-throwing, deduplicated failure reporter here (and use it for analogous consent-read failures).
let key = match RegKey::predef(hive).open_subkey(subkey) {
Ok(key) => key,
Err(e) if e.kind() == std::io::ErrorKind::NotFound => return PolicyValue::Absent,
Err(_) => return PolicyValue::Unreadable,
};
| log_execution(&ExecutionEvent { | ||
| backend, | ||
| sandbox_kind: backend, | ||
| exit_code: response.exit_code, | ||
| outcome, |
| # Run with --experimental to enable the telemetry section. The provider is | ||
| # registered during init (before execution); the MXC.Execution / MXC.Error | ||
| # events are emitted on completion, after the runner returns. The sandbox | ||
| # itself may fail (e.g. AppContainer prerequisites), but completion | ||
| # telemetry still fires for the failure, so events should be captured. |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2da373b9-0a51-4aa4-a968-14f05f27b5c7
48383e0 to
8440b1c
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated 1 comment.
Suppressed comments (6)
tests/scripts/run_telemetry_etw_smoke_test.ps1:142
- This launch no longer establishes a granted consent record, but
telemetry::is_enablednow requires persistedGrantedconsent. On a fresh profile or after opt-out, the provider never registers and this smoke test deterministically reaches “no events captured.” Arrange an isolated current-version grant and nonblocking policy before launching (or skip explicitly when that prerequisite cannot be established).
$proc = Start-Process -FilePath $wxcExe `
-ArgumentList "--debug", "--experimental", $configFile `
-PassThru -NoNewWindow -Wait
src/core/wxc_common/src/telemetry/events.rs:119
- This second ETW write also bypasses the live consent/policy gates. In particular, consent can be withdrawn between the paired execution and error writes; each event must independently revalidate authorization immediately before emission.
mxc_telemetry::log_error(
ctx.backend,
ctx.sandbox_kind,
src/core/wxc_common/src/telemetry/mod.rs:261
- Assigning both dimensions from
containment.wire_name()makesmxc.sandbox_kindandmxc.backendidentical, contrary to their contract. For example, caller intentprocessis mapped toprocesscontainer, while runtime dispatch then selects BaseContainer or an AppContainer tier; this code reports neither the requestedprocesskind nor the concrete selected backend. Preserve caller intent and propagate the selected backend/tier separately.
log_execution(&ExecutionEvent {
backend,
sandbox_kind: backend,
.github/copilot-instructions.md:152
- This command references a script that is not present in the repository, so following the documented test workflow fails immediately. Either add the consent smoke script in this PR or remove this entry until the later integration change introduces it.
tests\scripts\run_telemetry_consent_smoke_test.ps1 # Telemetry consent + policy CLI E2E (Windows; debug binary only)
sdk/dotnet/README.md:115
- This now claims that .NET policy and consent queries exist, but this PR adds no such .NET APIs; the stack description schedules them for PR #822. Keep the wording prospective until those surfaces are actually available.
for the stable registry contract and interaction rules. Policy and consent
queries fail closed rather than upgrading an unreadable device state into
collection.
docs/telemetry/telemetry.md:100
mxc.backendis defined as the concrete selected backend forMXC.Executionabove, but this edit gives the same field a different meaning forMXC.Error. Since paired events receive the same backend value, keep one stable field contract for collectors.
| `mxc.backend` | string | Containment backend name |
| mxc_telemetry::log_execution( | ||
| event.backend, | ||
| event.sandbox_kind, |
.github/copilot-instructions.md.Summary
Introduces the privacy foundation behind stable MXC telemetry: versioned consent resources, fail-closed consent persistence and status, withdrawal, the administrative deny-only policy ceiling, and balanced ETW provider lifetime behavior.
This is PR 2 of 5 and depends on the normative contract in PR 1.
Stack
Review only this PR's diff; prerequisite design context is in the PR above.