Uh oh!
There was an error while loading. Please reload this page.
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. |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
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.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
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.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
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"
);
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
469850f to
48383e0CompareResolve the schema-support and developer-guidance conflicts while preserving telemetry test support and smoke-test instructions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2da373b9-0a51-4aa4-a968-14f05f27b5c7
RamonArjona4
commented
Aug 24, 2026
Branden Bonaby (@bbonaby) Merged & resolved and verified suppressed comments, now just waiting for checks. |
| impl ConsentStatus { | ||
| /// Whether an explicit consent request may offer the canonical prompt. | ||
| pub fn needs_prompt(&self) -> bool { | ||
| matches!(self.effective_state, ConsentState::Undetermined) | ||
| } | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.
| for the stable registry contract and interaction rules. Policy and consent | ||
| queries fail closed rather than upgrading an unreadable device state into | ||
| collection. |
| /// 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. |
| 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, | ||
| }; | ||
| match key.get_value::<u32, _>(POLICY_VALUE_NAME) { | ||
| Ok(value) => PolicyValue::Value(value), | ||
| Err(e) if e.kind() == std::io::ErrorKind::NotFound => PolicyValue::Absent, | ||
| Err(_) => PolicyValue::Unreadable, |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2da373b9-0a51-4aa4-a968-14f05f27b5c7
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.
Suppressed comments (3)
Previously missed (1) — in code that hasn't changed since the last review.
.github/copilot-instructions.md:232
- This edit accidentally removes
docs/host-prep.mdfrom the core-reference list even though the document and host-preparation workflow are unchanged. Restore that bullet so the contributor instructions retain the existing setup reference.
- `docs/sandbox-policy/v1/policy.md` — sandbox policy v1 specification
- `docs/telemetry/telemetry.md` — telemetry overview; `docs/telemetry/telemetry-consent-design.md` (Windows-only consent design and per-SDK surface) and `docs/telemetry/telemetry-administrative-policy.md` (the MDM / Group Policy ceiling)
src/core/wxc_common/src/telemetry/consent.rs:942
- The lock-free read can still return a stale
Grantedafter withdrawal begins: a reader can observe the marker as absent here, then a writer creates the marker and replaces the record withdenied, while the reader finishes reading the old file. Becauseget_consent()feeds telemetry authorization, that permits an emission after a completed withdrawal. Coordinate reads with the writer lock or use an optimistic read that revalidates both the marker and record identity before accepting a grant, as required by the consent contract's reader-coordination/live-enforcement rules.
match marker_check(&marker_path) {
Ok(WithdrawalMarkerState::Absent) => {}
Ok(WithdrawalMarkerState::Pending) | Err(_) => {
return unreadable_status();
}
Ok(WithdrawalMarkerState::Stale) => {
if recover_stale(&path, &marker_path).is_err() {
return unreadable_status();
}
}
}
let data = match with_io_retry(|| read_bounded(&path)) {
sdk/dotnet/README.md:180
- The .NET SDK does not gain consent or policy query APIs in this PR—the stack assigns those surfaces to PR 5—so this present-tense claim is inaccurate for this revision. Keep the wording prospective until the SDK methods are introduced, or defer this README update to that PR.
for the stable registry contract and interaction rules. Policy and consent
queries fail closed rather than upgrading an unreadable device state into
collection.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2da373b9-0a51-4aa4-a968-14f05f27b5c7
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 3 comments.
Suppressed comments (6)
src/core/wxc_common/src/telemetry/consent.rs:990
- A current-version grant is accepted for any nonempty
promptLocale, even though this build can only present and persist the embeddeden-USresource. A corrupt/foreign record such aspromptLocale: "fr-FR"therefore authorizes collection despite being impossible to produce through MXC's presenter flow. Validate the locale against the embedded resource so unsupported provenance fails closed.
Some(CONSENT_RESOURCE_VERSION) if !record.prompt_locale.is_empty() => ConsentStatus {
tests/scripts/run_telemetry_etw_smoke_test.ps1:196
- This removes the only on-wire check for the newly added
mxc.sandbox_kindfield. The test sink bypassesmxc_telemetry, and the source-text test specifically checks onlyMXC.Error, soMXC.Executioncould omit this field while all remaining tests pass. Keep the field in the ETW smoke assertion.
$expectedFields = @('mxc.backend', 'mxc.exit_code', 'mxc.outcome', 'mxc.duration_ms')
.github/copilot-instructions.md:156
- This command documents
run_telemetry_consent_smoke_test.ps1, but that file is not present in this pull request, so contributors following the test instructions get a path-not-found error. Add the script here or defer this entry to the stack change that introduces it.
tests\scripts\run_telemetry_consent_smoke_test.ps1 # Telemetry consent + policy CLI E2E (Windows; debug binary only)
.github/copilot-instructions.md:231
- This adds a core-reference link to
docs/sandbox-policy/v1/policy.md, which is not present in this pull request, while replacing the existingdocs/host-prep.mdreference. Restore the existing reference and add the v1 entry only when its target lands.
- `docs/sandbox-policy/v1/policy.md` — sandbox policy v1 specification
sdk/dotnet/README.md:180
- The .NET SDK in this PR has no policy or consent query surface—the stack description assigns those APIs to PR #822—so this present-tense statement documents functionality consumers cannot call yet. Keep the prospective wording until the binding API lands.
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/consent.rs:1151
- The consent contract requires releasing the writer lock only after replacement is durable (
docs/telemetry/telemetry-consent-design.md:137-143).sync_all()flushes the temporary file, but this plain rename does not request write-through or flush the parent-directory metadata; the withdrawal marker is also removed immediately afterward. A withdrawal can therefore return success and then a power loss can restore the prior granted record. Use a Windows durable replace/write-through path (and durably order the withdrawal marker) before acknowledging the mutation.
if let Err(e) = with_io_retry(|| fs::rename(&tmp_path, &path)) {
remove_best_effort(&tmp_path);
return Err(format!("failed to finalize {}: {e}", path.display()));
}
clear_withdrawal_marker(&path)
| let data = match with_io_retry(|| read_bounded(&path)) { | ||
| Ok(data) => data, | ||
| Err(error) if error.kind() == std::io::ErrorKind::NotFound => { | ||
| return no_record_status() | ||
| } | ||
| Err(_) => return unreadable_status(), |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2da373b9-0a51-4aa4-a968-14f05f27b5c7
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2da373b9-0a51-4aa4-a968-14f05f27b5c7
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 16 changed files in this pull request and generated no new comments.
Suppressed comments (1)
tests/scripts/run_telemetry_etw_smoke_test.ps1:196
- This weakens the only test that inspects serialized ETW output: it drops
mxc.sandbox_kindand does not verify any of the new required Part A privacy fields. The source-text unit tests can still pass if TraceLogging fails to encode those fields, so keep the existing field and assert the new privacy metadata here as well.
$expectedFields = @('mxc.backend', 'mxc.exit_code', 'mxc.outcome', 'mxc.duration_ms')
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2da373b9-0a51-4aa4-a968-14f05f27b5c7
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 16 changed files in this pull request and generated no new comments.
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
src/core/wxc_common/src/telemetry/policy.rs:203
- Unrecognized DWORD values are silently collapsed to
Blocked. That hides a broken administrative deployment from operators, contrary to the repository rule that fail-closed failures must be reported once per distinct failure (.github/copilot-instructions.md:337). Report the invalid value through the same deduplicated failure reporter while still returningBlocked.
// Every other value — including `0` (off) and `1` (required-only,
// a category MXC does not emit) — denies. Unrecognized values deny
// too rather than being guessed at: fail closed.
PolicyValue::Value(_) => PolicyState::Blocked,
tests/scripts/run_telemetry_etw_smoke_test.ps1:196
- The smoke test no longer validates the newly added
mxc.sandbox_kindfield, so an ETW encoding/regression inMXC.Executioncan pass despite the documented collector contract. The source-text unit test only checks this field onMXC.Error; keep it in this end-to-end field list as well.
$expectedFields = @('mxc.backend', 'mxc.exit_code', 'mxc.outcome', 'mxc.duration_ms')
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2da373b9-0a51-4aa4-a968-14f05f27b5c7
Gudge (MGudgin)
left a comment
There was a problem hiding this comment.
Re-verified the existing findings against 55ce0a7. The stale-presenter, bounded-read, async-storage, and crash-durable withdrawal findings are addressed, and the targeted release consent and telemetry-provider tests pass. The remaining emit-vs-final-unregister test-coverage gap is Low and non-blocking.
Uh oh!
There was an error while loading. Please reload this page.
Resolves the `.github/copilot-instructions.md` conflict with #819 (telemetry consent). Both sides edited the thin-binary-crates bullet, but #819's edit to it was whitespace-only, so this keeps the branch's text (which documents `--available-backends`) and takes #819's new telemetry sections unchanged. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
.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.