Fix scoped ETW telemetry validation

[RamonArjona4]

📖 Description

This PR completes the Windows-local Microsoft.MXC ETW logging remediation. It separates ETW emission from local diagnostic sinks, corrects ETW payload semantics, strengthens payload and sink-gating tests, makes the ETW smoke test provider-aware, and documents the contract.

The scope is limited to logging/ETW emission, tests, performance, and documentation. It does not change network/DNS enforcement, fallback/DACL behavior, cleanup/kill behavior, execution policy, or add collectors/uploaders/services/daemons.

🔗 References

No linked issue.

🔍 Validation

• cargo fmt --all -- --check
• Targeted telemetry, policy identity, IsolationSession, and AppContainer tests
• cargo build --workspace
• PowerShell smoke-test script parsing

✅ Checklist

• Signed the Contributor License Agreement
• Linked to an issue
• Updated documentation (if applicable)
• Updated Copilot instructions (if build, architecture, or conventions changed)
• If this PR changes Cargo.lock, the dependency-feed-check check passes (see docs/pull-requests.md)

📋 Issue Type

• Bug fix
• Feature
• Task

Microsoft Reviewers: Open in CodeFlow

[azure-pipelines]

Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

[Copilot]

Pull request overview

This PR expands Windows ETW telemetry and separates it from local diagnostic audit logging.

Changes:

• Adds specialized lifecycle, policy, network, teardown, and rejection telemetry.
• Adds structured local audit records and policy hashing.
• Hardens diagnostic pipes and expands tests/documentation.

Reviewed changes

Copilot reviewed 32 out of 33 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
docs/diagnostics.md	Documents diagnostic sinks and pipe tokens.
docs/telemetry/telemetry.md	Defines ETW and local audit contracts.
src/Cargo.lock	Records SHA-256 dependency.
src/Cargo.toml	Adds workspace SHA-256 dependency.
src/backends/appcontainer/common/src/appcontainer_runner.rs	Adds AppContainer audit emissions.
src/backends/appcontainer/common/src/base_container_runner.rs	Adds BaseContainer lifecycle telemetry.
src/backends/appcontainer/common/src/dispatcher.rs	Records enforcement degradation.
src/backends/appcontainer/common/src/fallback_detector.rs	Adds bounded degradation reasons.
src/backends/appcontainer/common/src/job_object.rs	Exposes termination failures.
src/backends/appcontainer/common/src/network_manager.rs	Tracks network setup and cleanup outcomes.
src/backends/appcontainer/common/src/proxy_coordinator.rs	Reports proxy teardown activity.
src/backends/isolation_session/common/src/manager.rs	Adds process and teardown telemetry.
src/backends/isolation_session/common/src/one_shot.rs	Audits one-shot cleanup.
src/backends/isolation_session/common/src/state_aware.rs	Audits lifecycle cleanup and execution.
src/core/mxc_engine/src/dispatch.rs	Emits streaming policy hashes.
src/core/mxc_engine/src/lib.rs	Exports policy-hash logging.
src/core/mxc_engine/src/run.rs	Emits run policy and degradation records.
src/core/mxc_engine/src/state_aware.rs	Handles the new parse-error variant.
src/core/wxc/src/main.rs	Integrates rejection and state-aware auditing.
src/core/wxc_common/Cargo.toml	Adds SHA-256 dependency.
src/core/wxc_common/src/audit.rs	Defines structured local audit records.
src/core/wxc_common/src/config_deserialize.rs	Shares secret-field detection.
src/core/wxc_common/src/config_parser.rs	Classifies malformed one-shot JSON.
src/core/wxc_common/src/diagnostic.rs	Redacts diagnostics and secures pipe naming.
src/core/wxc_common/src/lib.rs	Exposes audit and policy identity modules.
src/core/wxc_common/src/logger.rs	Adds structured diagnostic sink routing.
src/core/wxc_common/src/models.rs	Adds canonical network strings.
src/core/wxc_common/src/policy_identity.rs	Implements canonical policy hashing.
src/core/wxc_common/src/telemetry/events.rs	Adds specialized telemetry wrappers.
src/core/wxc_common/src/telemetry/mod.rs	Exports specialized telemetry APIs.
src/mxc_telemetry/src/lib.rs	Implements specialized ETW events.
src/tools/mxc_diagnostic_console/src/main.rs	Hardens the pipe and parses audit envelopes.
tests/scripts/run_telemetry_etw_smoke_test.ps1	Makes ETW validation provider-aware.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 32 out of 33 changed files in this pull request and generated 1 comment.

Suppressed comments (6)

src/core/wxc/src/main.rs:428

• The mapping is not used for validation failures returned by mxc_engine::run_state_aware: after dispatch, the Err arm only calls log_state_aware_dispatch_error. Backend validation errors such as IsolationSession's invalid UPN/token therefore produce neither the local nor ETW ConfigRejected record, contradicting the stated state-aware M-ETW-7 coverage. Emit the rejection before emit_state_aware shuts the provider down for config/phase/ID error codes.

fn rejection_reason_for(error: &MxcError) -> RejectionReason {
match error.code {
MxcErrorCode::MalformedRequest => RejectionReason::SchemaViolation,
MxcErrorCode::MalformedId => RejectionReason::IdentityShapeInvalid,
MxcErrorCode::PolicyValidation => RejectionReason::UnsupportedFieldForBackend,
MxcErrorCode::UnsupportedContainment => RejectionReason::UnsupportedContainment,
MxcErrorCode::UnsupportedPhase => RejectionReason::UnsupportedPhase,
MxcErrorCode::BackendUnavailable
| MxcErrorCode::StaleId
| MxcErrorCode::NotProvisioned
| MxcErrorCode::NotStarted
| MxcErrorCode::AlreadyStarted
| MxcErrorCode::AlreadyStopped
| MxcErrorCode::BackendError => RejectionReason::RunnerUnavailable,
}

src/backends/appcontainer/common/src/appcontainer_runner.rs:1639

• The successful AppContainer teardown path never emits MXC.SandboxTornDown to ETW. This early return gates the whole remainder on a diagnostic sink, and unlike the pre-spawn teardown path there is no telemetry call afterward. Consequently T2/T3 runs miss M-ETW-5 even when the provider is active; compute the status first, emit ETW independently, then gate only the JSON audit record.

 if !self.audit_enabled() {
return;
}

src/core/wxc/src/main.rs:558

• State-aware MXC.PolicyHash is still tied exclusively to the local diagnostic sink. When telemetry is enabled without --log-file/the pipe, this block is skipped and no ETW policy-hash event is emitted, despite the documented M-ETW-3 state-aware coverage. Compute the hash when either sink is active and call telemetry::log_policy_hash when telemetry_active; only the JSON record should depend on has_diagnostic_sink().

 if logger.has_diagnostic_sink() {
let record = AuditEvent::new(AuditEventName::PolicyHash)
.str("backend", backend)
.str(
"policy_hash",
&wxc_common::policy_identity::state_aware_policy_hash(
&parsed.request,
backend,
phase,
phase_config,
),
)
.str("config_schema_version", &parsed.request.schema_version);
logger.log_audit_event(&record);
}

src/core/wxc/src/main.rs:370

• This ETW call is a no-op for every rejection that occurs during request loading (and for the state-aware command-override checks): the provider is initialized only later at lines 496-506 or 1261-1271. Thus malformed/schema-invalid requests never produce the documented MXC.ConfigRejected ETW event even when their payload contains experimental.telemetry.enabled=true. The telemetry opt-in must be resolved and the provider registered before these rejection sites, or the documented M-ETW-7 coverage must be narrowed.

 wxc_common::telemetry::log_config_rejected(
correlation_id,
backend,
reason.as_str(),
offending_field,
);

src/tools/mxc_diagnostic_console/src/main.rs:448

• The documented terminal hardening has no implementation: no function follows this comment, and DisplayEvent::Message still interpolates each untrusted pipe message directly into println! at lines 652-669. A client that knows the session token can therefore inject ESC/control sequences into the console. Add the described escaping function and apply it before all terminal rendering (while preserving the desired raw/escaped behavior for collection files).
  src/backends/appcontainer/common/src/proxy_coordinator.rs:481
• proxy_stopped reports only that the coordinator was active, not that cleanup succeeded. signal_process_cleanup can fail SetEvent or time out, and remove_loopback_exemption discards command failure, yet this returns true; callers then emit a successful teardown with proxy_stopped=true. Propagate those cleanup outcomes and include a proxy failure in the aggregate teardown status.

 pub fn stop(&mut self, logger: &mut Logger) -> bool {
let was_active = self.is_active();
signal_process_cleanup(
self.shim_cleanup_event.take(),
self.shim_process_handle.take(),
"winhttp-proxy-shim",
logger,
);
signal_process_cleanup(
self.test_proxy_cleanup_event.take(),
self.test_proxy_handle.take(),
"wxc-test-proxy",
logger,
);
self.proxy_address = None;
if let Some(path) = self.shim_ready_file_path.take() {
let _ = std::fs::remove_file(&path);
}
if let Some(path) = self.test_proxy_ready_file_path.take() {
let _ = std::fs::remove_file(&path);
}
if let Some(container_name) = self.loopback_container_name.take() {
remove_loopback_exemption(&container_name);
}
was_active

[Copilot]

Pull request overview

Copilot reviewed 32 out of 33 changed files in this pull request and generated no new comments.

Suppressed comments (10)

src/tools/mxc_diagnostic_console/src/main.rs:448

• The documented control-character hardening is not implemented here: this comment is attached to get_client_pid, while client_reader forwards parsed or raw client text and display_loop interpolates it directly into println!. An ESC byte from a client can therefore still execute terminal control sequences. Add the described renderer and apply it before console output (while retaining an appropriate representation for collected logs).
  src/core/wxc_common/src/audit.rs:547
• These shape checks do not prove that MXC minted the identity. containerId is copied directly from caller configuration, and the ProcessContainer call sites pass it here, so a caller can choose sandbox-0123456789abcdef or iso:ticket-123 and have that value emitted verbatim despite the stated rule that caller-supplied IDs are always redacted. Preserve IDs only when the call site can provide trusted provenance; sanitize arbitrary containerId values unconditionally.

 let mxc_opaque = identity
.split_once(':')
.map(|(prefix, token)| {
matches!(prefix, "iso" | "wsb")
&& !token.is_empty()
&& token
.bytes()
.all(|b| b.is_ascii_alphanumeric() || b == b'-' || b == b'_' || b == b'.')
})
.unwrap_or(false);
if mxc_opaque {
identity

src/backends/appcontainer/common/src/proxy_coordinator.rs:481

• This returns whether the proxy was active before cleanup, not whether it was stopped successfully. signal_process_cleanup can time out or fail to signal, and loopback-exemption removal discards its result, yet callers now record this value as proxy_stopped=true and may report a successful teardown. Return and aggregate the actual cleanup outcomes instead of was_active.

 pub fn stop(&mut self, logger: &mut Logger) -> bool {
let was_active = self.is_active();
signal_process_cleanup(
self.shim_cleanup_event.take(),
self.shim_process_handle.take(),
"winhttp-proxy-shim",
logger,
);
signal_process_cleanup(
self.test_proxy_cleanup_event.take(),
self.test_proxy_handle.take(),
"wxc-test-proxy",
logger,
);
self.proxy_address = None;
if let Some(path) = self.shim_ready_file_path.take() {
let _ = std::fs::remove_file(&path);
}
if let Some(path) = self.test_proxy_ready_file_path.take() {
let _ = std::fs::remove_file(&path);
}
if let Some(container_name) = self.loopback_container_name.take() {
remove_loopback_exemption(&container_name);
}
was_active

src/backends/appcontainer/common/src/appcontainer_runner.rs:1816

• ProcessExited is emitted only from blocking wait(). The public try_wait() can return the terminal exit code, and the FFI explicitly recommends polling it for cancellable waits; a caller that observes Some(code) and frees the handle never calls this branch, so the required process-outcome event is lost. Move once-only terminal-event emission into logic shared by try_wait() and wait().

 if !self.kill_requested {
wxc_common::telemetry::log_process_event(
wxc_common::telemetry::ProcessEventKind::Exited,
sanitize_identity(&self.identity),
self.pid,
wxc_common::telemetry::ProcessEventData::ExitCode(exit_code),
);

src/backends/appcontainer/common/src/base_container_runner.rs:2735

• ProcessExited is emitted only from blocking wait(). The public try_wait() can return the terminal exit code, and the FFI explicitly recommends polling it for cancellable waits; a caller that observes Some(code) and frees the handle never calls this branch, so the required process-outcome event is lost. Move once-only terminal-event emission into logic shared by try_wait() and wait().

 if !self.kill_requested {
wxc_common::telemetry::log_process_event(
wxc_common::telemetry::ProcessEventKind::Exited,
sanitize_identity(&self.identity),
self.pid,
wxc_common::telemetry::ProcessEventData::ExitCode(exit_code),
);

src/mxc_telemetry/src/lib.rs:316

• The specialized kill-failure payload never carries the failure itself: callers pass terminate_job_object/terminate_process here, so mxc.error_type describes the attempted method while the Win32/HRESULT error code is emitted only to the separate JSON audit sink. This does not satisfy the documented M-ETW-1 error field. Add a bounded numeric error-code field and plumb the actual error through this API.
  docs/telemetry/telemetry.md:98
• This row overstates ETW coverage for configuration rejection. wxc-exec initializes telemetry only after load_mxc_request_with_options succeeds, so malformed JSON and schema failures handled by the preceding error arms call an unregistered provider and reach only the local diagnostic sink. Clarify that M-ETW-7 covers post-parse rejections, or provide a consent-preserving initialization path for pre-parse failures.

| Configuration rejection (M-ETW-7) | `MXC.ConfigRejected` | `correlationId` or identity, bounded reason/error code, offending field path | ProcessContainer and IsolationSession validation paths |

tests/scripts/run_telemetry_etw_smoke_test.ps1:190

• These are two independent whole-document predicates, so they do not establish that any counted <Event> belongs to this provider: the GUID may appear in tracer metadata while $eventCount includes a different event. Parse the XML and count only event nodes whose System/Provider/@Guid matches $providerGuid; use that filtered count for the pass condition.
  src/core/wxc_common/src/telemetry/events.rs:402
• The new process-event test sink discards the event-specific payload (exit_code, timeout_ms, or kill error/type), so the added test can pass even if those values are wired to the wrong ETW event. Capture ProcessEventData in CapturedRequirement (or a typed captured-process record) and assert each value, including the kill-failure payload.

 fields: vec![
("identity".to_owned(), identity.to_owned()),
("process_id".to_owned(), process_id.to_string()),
],

docs/diagnostics.md:112

• The documented SID-less fallback cannot work on the server: create_pipe_instance now returns an error when current_user_sid() is None, so the console exits instead of creating \\.\pipe\mxc-diagnostics-{TOKEN}. Remove this fallback claim or implement a secure SID-resolution fallback consistently in both name construction and the pipe ACL.

authenticates the pipe. (If the SID cannot be resolved, the name degrades to
`\\.\pipe\mxc-diagnostics-{TOKEN}`.) Because the token is part of the name, the

[Copilot]

Pull request overview

Copilot reviewed 32 out of 33 changed files in this pull request and generated no new comments.

Suppressed comments (9)

src/backends/appcontainer/common/src/proxy_coordinator.rs:481

• proxy_stopped now means only “a proxy was active,” not “it stopped successfully.” Both cleanup signals can fail or time out, and loopback-exemption removal discards its result, yet callers report this true value in teardown telemetry. Return and aggregate the actual cleanup outcomes so failed cleanup is not reported as released.

 was_active

src/core/wxc/src/main.rs:574

• This emits the caller-provided sandboxId before backend dispatch validates its encoded payload. A short forged value such as iso:alice passes sanitize_identity and reaches ETW verbatim even though the backend later rejects it, violating the documented no-caller-identifiers contract. Use a redacted marker until backend-specific ID validation succeeds, or move identity-bearing emission after validation.

 let identity = state_aware_policy_identity(parsed.sandbox_id.as_deref());
wxc_common::telemetry::log_policy_hash(
&identity,
&policy_hash,
&parsed.request.schema_version,
);

src/core/wxc_common/src/telemetry/events.rs:402

• The test sink drops the event-specific payload, so these tests cannot detect swapped or incorrect exit_code, timeout_ms, or kill-failure values—the payload semantics this PR is intended to validate. Capture ProcessEventData in the test record and assert each concrete field/value.

 .push(CapturedRequirement {
name: name.to_owned(),
fields: vec![
("identity".to_owned(), identity.to_owned()),
("process_id".to_owned(), process_id.to_string()),
],

tests/scripts/run_telemetry_etw_smoke_test.ps1:190

• These independent whole-document checks do not prove that any counted <Event> belongs to Microsoft.MXC: the GUID may occur in tracer/session metadata while $eventCount counts another event. Parse the XML and count only event nodes whose System/Provider/@Guid matches $providerGuid, then use that filtered count as the pass condition.
  docs/diagnostics.md:112
• The documented SID-less fallback cannot work for the console: create_pipe_instance returns an error when current_user_sid() is unavailable because it needs that SID for the DACL. Remove this fallback claim or implement a secure, consistent fallback in both naming and ACL creation.

authenticates the pipe. (If the SID cannot be resolved, the name degrades to
`\\.\pipe\mxc-diagnostics-{TOKEN}`.) Because the token is part of the name, the

docs/diagnostics.md:168

• Requiring High integrity here conflicts with the same document's claim that pipe messages work without elevation: a non-elevated console runs at Medium integrity and Logger::verify_server_integrity rejects it. Either permit a same-user Medium-integrity server under the new token/ACL model or continue requiring elevation in the quick start and pipe documentation.

- Clients verify the pipe server runs at High integrity level or above before
sending data

src/core/wxc_common/src/audit.rs:45

• Correct the malformed doc text.

/// Closed set of audit record names. The `mxc.` prefix namespaces the record/// against unrelated lines sharing the same sink.

src/backends/appcontainer/common/src/appcontainer_runner.rs:1811

• ProcessExited is emitted only by blocking wait(). Public SDK/FFI consumers can observe completion via try_wait() and then drop/free the handle, so that valid path produces no process-outcome event; repeated wait() calls can also duplicate it. Route terminal observation through a shared once-only emitter used by both try_wait() and wait().

 wxc_common::telemetry::log_process_event(

src/backends/appcontainer/common/src/base_container_runner.rs:2730

• ProcessExited is emitted only by blocking wait(). Public SDK/FFI consumers can observe completion via try_wait() and then drop/free the handle, so that valid path produces no process-outcome event; repeated wait() calls can also duplicate it. Route terminal observation through a shared once-only emitter used by both try_wait() and wait().

 wxc_common::telemetry::log_process_event(

[Copilot]

Pull request overview

Copilot reviewed 35 out of 36 changed files in this pull request and generated 1 comment.

Suppressed comments (9)

src/tools/mxc_diagnostic_console/src/main.rs:449

• The documented hardening is not implemented: these lines are rustdoc on get_client_pid, and DisplayEvent::Message text is still interpolated directly into println! later in this file. A diagnostic client can therefore inject ESC/control sequences into the operator's terminal. Add the described sanitizer and apply it to untrusted message text before terminal rendering.
  docs/diagnostics.md:164
• A token in the pipe name is not primary access control because same-user processes can enumerate the named-pipe namespace and discover the endpoint without guessing. Until an in-band authentication mechanism is added, describe this only as session separation and identify the user-restricted DACL as the actual access control.

- `MXC_DIAG_PIPE_TOKEN` is the primary access control: it is mixed into the pipe
name, so a process that does not know the token cannot guess the endpoint.
`wxc-exec` refuses to connect at all when no valid token is set

docs/diagnostics.md:111

• The token separates concurrent sessions but cannot authenticate an enumerable named-pipe endpoint. This should not promise authentication until the client/server protocol verifies possession in-band.

the token both separates concurrent sessions for the *same* user and
authenticates the pipe. (If the SID cannot be resolved, the name degrades to

src/core/wxc/src/main.rs:395

• This assumes the parser path is bounded schema vocabulary, but unknown-field errors include the caller-controlled key itself (for example, existing parser tests produce process.bogus). An attacker can therefore put arbitrary/high-cardinality text, including sensitive text, into mxc.offending_field and the local audit record. Emit only validated known paths (or omit unknown keys), and enforce a length bound before telemetry emission.

fn offending_field_from_message(message: &str) -> &str {
const PREFIX: &str = "Invalid configuration at `";
let Some(start) = message.find(PREFIX) else {
return "";
};
let field = &message[start + PREFIX.len()..];
field.split('`').next().unwrap_or("")

src/backends/appcontainer/common/src/appcontainer_runner.rs:1815

• Process outcome emission is tied only to wait() and has no once guard. The public Rust SDK can observe completion through try_wait() and then drop the handle, producing no ProcessExited; calling wait() repeatedly produces duplicate events. Record the first terminal observation in shared state and use the same helper from both wait() and try_wait().

 let exit_code = code as i32;
if !self.kill_requested {
wxc_common::telemetry::log_process_event(
sanitize_identity(&self.identity),
self.pid,
wxc_common::telemetry::ProcessEvent::Exited(exit_code),
);
}

src/backends/appcontainer/common/src/base_container_runner.rs:2734

• Process outcome emission is tied only to wait() and has no once guard. The public Rust SDK can observe completion through try_wait() and then drop the handle, producing no ProcessExited; calling wait() repeatedly produces duplicate events. Record the first terminal observation in shared state and use the same helper from both wait() and try_wait().

 let exit_code = code as i32;
if !self.kill_requested {
wxc_common::telemetry::log_process_event(
sanitize_identity(&self.identity),
self.pid,
wxc_common::telemetry::ProcessEvent::Exited(exit_code),
);
}

README.md:255

• Official grouped builds route every event using MXC_EVENT_KEYWORD, including the newly added process, policy, network, teardown, and rejection events—not only MXC.Execution and MXC.Error. This understates what is Microsoft-routed when users opt in; update the disclosure to cover all provider events and their bounded payloads.

Official/shipped Microsoft builds set a TraceLogging provider group GUID at build time and route `MXC.Execution` and `MXC.Error` events to Microsoft through the UTC pipeline when telemetry is enabled — that same build-time setting also selects the correct Measures keyword and Product-and-Service-Usage privacy tag for the events, so telemetry routing and event classification always agree. **Local and open-source builds send nothing to Microsoft by default** — the public source ships without a provider group GUID, so events are emitted to the local ETW subsystem only, use a provider-local keyword with no UTC meaning, and carry no privacy classification tag, and are not routed to any Microsoft collection pipeline. Internal builds that set the `MXC_TELEMETRY_PROVIDER_GROUP_GUID` environment variable at build time enable the Microsoft-routed path.

src/core/wxc_common/src/logger.rs:336

• write_all is not guaranteed to issue one OS write; it retries after partial writes. Because cloned logger handles and separate MXC processes append without a shared lock, another writer can interleave between those retries and corrupt the claimed one-record-per-line stream. Use a cross-process lock or a bounded single WriteFile operation whose partial-write failure is handled without retrying into the same record.

 let _ = file.write_all(out.as_bytes());

docs/diagnostics.md:34

• The pipe name token selects a session but does not authenticate it because same-user processes can enumerate named pipes and learn the token. Calling it authentication overstates the security guarantee.

This issue also appears in the following locations of the same file:

• line 110
• line 162

| Env var | `MXC_DIAG_PIPE_TOKEN=<token>` | **Required** for pipe output. Selects the per-session pipe and authenticates it; use the same token for the console and `wxc-exec` |

[Copilot]

Pull request overview

Copilot reviewed 35 out of 36 changed files in this pull request and generated no new comments.

Suppressed comments (4)

src/tools/mxc_diagnostic_console/src/main.rs:439

• The new terminal-hardening documentation is not backed by any implementation: this doc block is attached to get_client_pid, and DisplayEvent::Message still interpolates each client-controlled line directly into println! at lines 654–668. An ESC byte can therefore still inject terminal control sequences. Please add the described renderer and apply it before every TTY write (while retaining the original text for plain log files).
  src/backends/appcontainer/common/src/appcontainer_runner.rs:1823
• ProcessExited is emitted only from wait(), but the public Rust SDK and C FFI expose try_wait() as a terminal polling path. A streaming caller that stops after try_wait() returns Some(code) never reaches this block, so the promised process-outcome event is missing. Route both try_wait() and wait() through an idempotent terminal-event helper so polling emits exactly one event.

 if !self.kill_requested {
wxc_common::telemetry::log_process_event(
sanitize_identity(&self.identity),
self.pid,
wxc_common::telemetry::ProcessEvent::Exited(exit_code),
);

src/backends/appcontainer/common/src/base_container_runner.rs:2764

• ProcessExited is emitted only from wait(), while SDK/FFI streaming callers may finish by polling try_wait() until it returns Some(code). That path currently returns the exit code without emitting this event, leaving successful streaming runs without the required outcome record. Please use a shared idempotent helper from both terminal paths.

 if !self.kill_requested {
wxc_common::telemetry::log_process_event(
sanitize_identity(&self.identity),
self.pid,
wxc_common::telemetry::ProcessEvent::Exited(exit_code),
);

README.md:255

• This “What official builds send” paragraph still names only MXC.Execution and MXC.Error, but this PR adds several events to the same provider (Process*, PolicyHash, network policy, teardown, and config rejection), and official builds route those too. Please update the disclosure or link the complete inventory in docs/telemetry/telemetry.md; otherwise the README understates the data sent by official builds.

Official/shipped Microsoft builds set a TraceLogging provider group GUID at build time and route `MXC.Execution` and `MXC.Error` events to Microsoft through the UTC pipeline when telemetry is enabled — that same build-time setting also selects the correct Measures keyword and Product-and-Service-Usage privacy tag for the events, so telemetry routing and event classification always agree. **Local and open-source builds send nothing to Microsoft by default** — the public source ships without a provider group GUID, so events are emitted to the local ETW subsystem only, use a provider-local keyword with no UTC meaning, and carry no privacy classification tag, and are not routed to any Microsoft collection pipeline. Internal builds that set the `MXC_TELEMETRY_PROVIDER_GROUP_GUID` environment variable at build time enable the Microsoft-routed path.

[Copilot]

Pull request overview

Copilot reviewed 35 out of 36 changed files in this pull request and generated no new comments.

Suppressed comments (6)

src/tools/mxc_diagnostic_console/src/main.rs:439

• This rustdoc is attached to get_client_pid, but it describes a terminal-message renderer that does not exist in this file. As written, generated documentation falsely claims control-character and Unicode escaping; remove the stale block (or implement and document the renderer separately).
  src/mxc_telemetry/src/lib.rs:316
• Use Warning here. The adjacent MXC.Error contract reserves Error/Critical for product faults that feed reliability alerting, while ProcessKillFailed is best-effort and may be caused by a normal process-exit race. Classifying that expected race as Error produces false reliability signals.
  src/mxc_telemetry/src/lib.rs:327
• This field is not receiving an error type: every new caller passes a kill method such as terminate_job_object or terminate_process. Consumers therefore get an undocumented value in mxc.error_type and no error reason/code, despite the M-ETW-1 contract requiring an error payload. Carry a bounded kill method plus numeric error code, or pass an actual bounded error category through ProcessEvent and its callers.
  src/core/wxc_common/src/policy_identity.rs:105
• phase_config is the raw, permissive experimental JSON, so hashing it wholesale and removing only secret-looking key names breaks this module's explicit allow-list/privacy contract. For example, IsolationSession appId may be any caller string and is explicitly not interpreted or enforced, yet it affects this deterministic hash; unknown non-secret-named fields do too. Project only typed enforcement fields and exclude appId/unknown fields so arbitrary caller data cannot become a confirmation oracle.

 let mut config = phase_config.cloned().unwrap_or(Value::Null);
strip_keys(&mut config, &["user", "upn"]);

src/core/wxc_common/src/policy_identity.rs:110

• Including the lifecycle phase makes one sandbox's PolicyHash differ across provision/start/exec/stop even when its enforcement policy is unchanged. The phase is already carried by lifecycle telemetry; exclude it from the policy projection so this remains a stable identity for the effective policy as documented.

 "phase": phase,

src/backends/appcontainer/common/src/base_container_runner.rs:2759

• kill_requested remains false after this normal-exit event because BaseContainer later calls the internal terminate_and_reap() rather than self.kill(). Since public wait() only borrows the handle, a second call emits ProcessExited again; after a timeout, a later call emits an additional exit event. Track whether a terminal event has already been emitted (or mark both success and timeout outcomes consumed) so each process produces one outcome.

 if !self.kill_requested {

[Copilot]

Pull request overview

Copilot reviewed 37 out of 38 changed files in this pull request and generated no new comments.

Suppressed comments (6)

src/core/wxc_common/src/telemetry/events.rs:391

• The real MXC.ProcessTimedOut event writes mxc.timeout_ms (src/mxc_telemetry/src/lib.rs:306), not TimeoutMs. Keeping a different test-sink schema masks precisely the ETW payload mismatch this PR intends to catch.
  src/core/wxc_common/src/policy_identity.rs:170
• containerId can be caller-supplied identity material (including a UPN), yet it is included verbatim in the deterministic, unkeyed policy hash that official builds may emit to telemetry. This reintroduces the dictionary/confirmation oracle that sanitize_identity explicitly avoids for user identifiers: an observer can hash candidate IDs with otherwise-known defaults and compare them. Exclude or irreversibly sanitize identity-bearing IDs before hashing, or use a keyed construction whose key is not emitted.
  src/core/wxc_common/src/telemetry/events.rs:383
• The production provider emits this field as mxc.exit_code (src/mxc_telemetry/src/lib.rs:285). Using ExitCode in the capture sink lets schema tests pass against a field that no real ETW event contains, so the new payload validation does not validate the production contract.

This issue also appears on line 391 of the same file.
CODE_REVIEW_TRACELOGGING_COMPLIANCE.md:93

• This assertion is factually reversed: mxc_telemetry declares mxc.exit_code and mxc.timeout_ms (src/mxc_telemetry/src/lib.rs:285,306). As written, this compliance record documents and endorses a schema that the provider does not emit; update the assessment after aligning the test sink with the provider.

**Rationale:**
1. **Schema Consistency** — Event field names must match the emitting provider's schema. The ETW provider (implemented in `mxc_telemetry`) declares these fields in PascalCase; the test sink was using snake_case, which masked a mismatch.
2. **Type Safety in Practice** — This fix ensures that when real ETW consumers parse the event payload, field names match what the schema specifies.
3. **Privacy and Data Tagging Readiness** — Consistent field naming is prerequisite for applying privacy tags (PDT_*) at schema-generation time.
4. **Cross-Consumer Compatibility** — Any Kusto query or downstream telemetry pipeline depending on `ExitCode` (not `exit_code`) now sees consistent data.

CODE_REVIEW_TRACELOGGING_COMPLIANCE.md:300

• There is no Linux/LTTng backend: the entire non-Windows provider is implemented as no-op stubs returning inactive (src/mxc_telemetry/src/lib.rs:456-520). The claims that events are emitted identically across platforms and only the transport differs are therefore incorrect and should describe this as a Windows-only ETW implementation.

**Pattern Used:**
The `mxc_telemetry` crate is Rust-native and exposes the same API across Windows, Linux, and potentially macOS, using platform-specific backends:
- Windows: Native ETW via `tracelogging` crate bindings
- Linux: LTTng via `tracelogging` crate support
- macOS: Future support via abstraction layer
**Compliance Assessment:** ✅ **EXCELLENT**
**Rationale:**
1. **Cross-Platform Consistency** — Events are emitted identically regardless of host OS; only the transport differs (ETW vs LTTng vs syslog).
2. **Asimov Best Practice #10** — Cross-Platform Support: "Abstract tracelogging APIs so that events can be emitted the same way from Windows, Linux, or other environments."

docs/telemetry/telemetry.md:434

• The IsolationSession teardown implementation never emits client_unregistered; its record contains session_stopped and agent_user_deprovisioned only. Listing this field as part of the contract makes consumers expect data that cannot be produced.

| `mxc.SandboxTornDown` | Per-run resources released, once per handle | ProcessContainer: `backend`, `identity`, `tier`, `pid`, `status`, `firewall_rules_removed`, `firewall_removal_ok`, `bfs_removed`, `proxy_stopped`, `preserve_policy`, `container_released`, `skip_reason`. IsolationSession: `backend`, `identity`, `phase`, `status`, `session_stopped`, `agent_user_deprovisioned`, `client_unregistered` |

[Gudge (MGudgin)]

Summary

I verified these findings against PR #791 at head 424a486 before filing. The review covers 4 High, 13 Medium, and 4 Low findings; 20 are anchored to added lines and one PR-description finding is recorded below.

The branch is behind its current main base: baseRefOid=aa05d77, while the PR merge-base is 3a5d0f1. This review uses GitHub's actual three-dot PR range. The local and GitHub diffs both contain 10,436 lines (their normalized byte counts differ by 76 bytes from headers/encoding).

Verified clean, with receipts

• All 16 unique cited paths have non-empty git diff --numstat output in this PR. No finding relies on a byte-identical out-of-diff file.
• Every inline comment below was checked against the captured diff and lands on an added (+) line.
• Runner test counts increased (appcontainer_runner.rs: 29 to 34; IsolationSession manager.rs: 1 to 4), but neither runner test module calls the telemetry capture sink; the integration-coverage finding is therefore about the new wiring, not an absence of all tests.
• Cross-platform parity was reviewed separately and no accidental shared-build/API asymmetry was found.

Attribution adjustments

• The config file reread predates this PR; the performance comment is limited to the newly added ungated redaction and full-request rendering.
• The teardown buffer logger predates this PR; the maintainability comment is framed as a gap in the new diagnostic-sink preservation fix.
• initialize_policy predates this PR; the simplicity comment targets the newly added describe_policy copy and its divergent decision.

Finding in the PR description

Medium (proportionality) - The stated scope understates changes to enforcement paths. The description says network/DNS enforcement and fallback/DACL behavior are unchanged, but this PR rewrites control flow in fallback_detector.rs and network_manager.rs to collect telemetry fields. Final behavior appears restored, but commit bf8f8eb was required to repair a BaseContainer denied-path DACL regression introduced during that refactor. Please update the PR description to name these enforcement-path changes so the appropriate reviewers are engaged.

Verified pre-existing - not attributed to this PR

No byte-identical out-of-diff finding is being charged to this PR. The three pre-existing structures noted above are included only where the new code adds an ungated cost, leaves a gap in the new fix, or duplicates an existing decision.

[Gudge (MGudgin)]

Low (supply-chain) - Core use of sha2 0.10 further entrenches duplicate versions.

This new direct workspace use expands sha2 0.10 into wxc_common while sha2 0.11 is already present transitively. No new crate or MSRV regression is introduced, but the binary now carries the older implementation more broadly.

Fix: Track unification on one supported sha2 version when dependencies retaining 0.10 can move.

[RamonArjona4]

Thanks for flagging this. Confirmed via cargo tree -i sha2: sha2 0.10.9 is pulled in transitively by several existing backends (lxc_common, bwrap_common, etc.) independent of this PR, and sha2 0.11.0 is present as a separate transitive dependency of an unrelated crate. This PR's new direct wxc_common -> sha2 0.10 dependency does not introduce a new duplicate pair -- it adds one more consumer of a version that was already duplicated on main before this change.

Unifying on a single sha2 version workspace-wide is a legitimate follow-up, but it requires coordinating a version bump across multiple unrelated backend crates that do not otherwise touch this PR's ETW/logging scope, so I'm deferring it rather than bundling it into this change. Tracking it as a separate housekeeping item is the right call here; no code change is being made in this PR for this comment.

[Copilot]

Pull request overview

Copilot reviewed 35 out of 36 changed files in this pull request and generated no new comments.

Suppressed comments (4)

src/tools/mxc_diagnostic_console/src/main.rs:557

• Re-serializing record through serde_json::Value loses the builder's field order (the default map is key-sorted), so pipe-delivered audit lines no longer begin with {"event":...} as the documented classifier requires. Preserve the raw JSON slice instead of converting the record back from Value.
  src/backends/appcontainer/common/src/base_container_runner.rs:2759
• A successful external kill() sets this flag, so the subsequent wait() suppresses the only terminal ProcessExited record. The AppContainer path in this PR explicitly fixes the same gap; BaseContainer cancellations (and failed kills followed by natural exit) must also emit the observed exit outcome. Remove this suppression and the now-unnecessary kill_requested state.

 if !self.kill_requested {

docs/diagnostics.md:49

• The documented rule does not match validation: four distinct characters have at most 2 bits/character and are rejected by the 3-bit Shannon threshold. Document the actual entropy threshold so users can diagnose tokens that the console refuses.

- at least **4 distinct** non-`-` characters.

src/backends/appcontainer/common/src/proxy_coordinator.rs:481

• This returns whether the proxy was active before cleanup, not whether it was stopped. signal_process_cleanup can fail SetEvent or time out, and remove_loopback_exemption discards its result, yet callers will emit proxy_stopped=true; propagate the cleanup outcomes so teardown telemetry does not report a failed release as successful.

 was_active

[Gudge (MGudgin)]

Re-reviewed current head b60e655.

Confirmed the ETW payload, shared telemetry test lock, pipe SQOS, identity redaction, no-sink performance gates, allocation fixes, teardown diagnostics, RAII cleanup, artifact removal, policy-plan reuse, entropy validation, and named access-mask fixes. The targeted verification suite passed 38 tests with 0 failures across wxc_common, appcontainer_common, and mxc_telemetry.

The sha2 version-unification follow-up is tracked in #844.

The remaining BaseContainer cancellation-outcome parity, broader runner-to-ETW integration coverage, and PR-description scope wording are non-blocking observations for this approval.

[Copilot]

🟡 Changes recommended

Token validation permits weak values, proxy cleanup telemetry can be inaccurate, and several public instructions no longer match runtime behavior.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

docs/diagnostics.md:49

• These documented acceptance rules do not match has_sufficient_entropy: four distinct characters can have only 2 bits/character and are rejected, while the implementation requires at least 3 bits/character. The length rule should also clarify that separators do not count once the validator is hardened; otherwise users can generate tokens that the documentation says are valid but MXC silently treats as absent.

- at least **32 characters**,
- only ASCII hex digits (`0-9`, `a-f`, `A-F`) and `-`,
- at least **4 distinct** non-`-` characters.

• Files reviewed: 34/35 changed files
• Comments generated: 4
• Review effort level: Balanced

[Copilot]

🔵 Needs a closer look

Identity correlation, lifecycle event completeness, cleanup reporting, and documentation contain unresolved correctness issues.

Review details

Suppressed comments (7)

Previously missed (5) — in code that hasn't changed since the last review.

src/backends/appcontainer/common/src/appcontainer_runner.rs:2103

• Process-exit emission occurs only in wait(). A streaming caller can observe Some(exit_code) through the public Sandbox::try_wait() API and then drop the handle, producing no ProcessExited event; repeated wait() calls can also emit duplicates. Record the terminal outcome exactly once from both observation paths.
  src/backends/appcontainer/common/src/base_container_runner.rs:2906
• Process-exit emission occurs only in wait(). A streaming caller can observe Some(exit_code) through the public Sandbox::try_wait() API and then drop the handle, producing no ProcessExited event; repeated wait() calls can also emit duplicates. Record the terminal outcome exactly once from both observation paths.
  src/core/mxc_engine/src/run.rs:109
• The ETW identity is derived from the requested container_id, but BaseContainer replaces that identity at launch (for example, an empty/default ID becomes a generated sandbox-… identity in base_container_runner.rs:1271-1319). The resulting MXC.PolicyHash carries CLI while process and teardown events carry the generated identity, so these events cannot be joined as documented. Emit the hash after runner identity resolution or pass the resolved identity into this emission.
  docs/diagnostics.md:49
• This rule does not match the implementation: is_valid_pipe_token requires at least 3 bits/character of measured Shannon entropy, not four distinct characters (a balanced four-character token has only 2 bits and is rejected). Document the actual threshold so users can diagnose rejected tokens.
  docs/diagnostics.md:94
• Calling these events “local ETW only” is false for official builds: the same PR documents that a configured provider group routes them through UTC. Clarify the build-dependent behavior so the diagnostics guide does not contradict the telemetry contract.

src/backends/appcontainer/common/src/proxy_coordinator.rs:457

• was_active does not establish that the proxy stopped. signal_process_cleanup only logs SetEvent and five-second wait failures, so this method returns true even when a proxy process remains alive; teardown records then incorrectly report proxy_stopped=true. Propagate and aggregate the actual cleanup results instead.

 let was_active = self.is_active();

docs/diagnostics.md:20

• This Quick Start no longer says to elevate the console, but the client accepts a pipe server only at High integrity or above (logger.rs:191-301). Following these commands in a normal terminal therefore makes wxc-exec reject the console, contradicting the later claim that pipe messages work without elevation. Either require an elevated console throughout the documentation or relax the client requirement.

# Terminal 1: generate one token and start the diagnostic console
$env:MXC_DIAG_PIPE_TOKEN = [guid]::NewGuid().ToString("N")
$env:MXC_DIAG_PIPE_TOKEN
mxc-diagnostic-console.exe

• Files reviewed: 34/35 changed files
• Comments generated: 0 new
• Review effort level: Balanced