Update Learning Mode to V2 API contracts - #739
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3a8860a9-bb20-48ef-91a1-3da8e34b92fb
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3a8860a9-bb20-48ef-91a1-3da8e34b92fb
Document the official-only exports and attribute-based process launch after rebasing the V2 contract changes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3a8860a9-bb20-48ef-91a1-3da8e34b92fb
Clarify that captureDenials now decodes ETLs and returns output metadata automatically, while lm_analyze remains a developer diagnostic. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3a8860a9-bb20-48ef-91a1-3da8e34b92fb
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
Updates Windows captureDenials to the official V2 Learning Mode and PSEC contracts.
Changes:
- Implements HRESULT-based Start/Stop and explicit Close lifecycles.
- Adds generated PSEC 1.0 bindings and capture-specific policy encoding.
- Bypasses legacy SBOX fallback and documents unsupported policy combinations.
Reviewed changes
Copilot reviewed 16 out of 32 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
.github/copilot-instructions.md |
Documents V2 lifecycle architecture. |
docs/learning-mode/capabilities.md |
Documents capture restrictions. |
docs/schema.md |
Updates schema guidance. |
external/windows-sdk/ProcessSecurityEnvironment.fbs |
Adds official PSEC schema. |
src/Cargo.lock |
Records PSEC crate dependencies. |
src/Cargo.toml |
Registers the generated crate. |
src/backends/appcontainer/common/Cargo.toml |
Adds PSEC dependency. |
src/backends/appcontainer/common/src/base_container_runner.rs |
Builds and launches PSEC capture environments. |
src/backends/appcontainer/common/src/dispatcher.rs |
Routes capture directly to V2. |
src/backends/learning_mode/windows/Cargo.toml |
Updates example dependencies. |
src/backends/learning_mode/windows/examples/lm_analyze.rs |
Clarifies diagnostic usage. |
src/backends/learning_mode/windows/examples/lm_capture.rs |
Exercises PSEC V2 capture. |
src/backends/learning_mode/windows/examples/lm_probe.rs |
Probes V2 exports. |
src/backends/learning_mode/windows/src/ffi.rs |
Implements V2 trace ABI. |
src/backends/learning_mode/windows/src/lib.rs |
Updates public contracts and errors. |
src/backends/learning_mode/windows/src/lifecycle.rs |
Implements Stop/Close sequencing. |
src/backends/learning_mode/windows/src/secenv.rs |
Implements V2 PSEC ABI. |
src/core/generated/process_security_environment_specification/Cargo.toml |
Defines generated crate. |
src/core/generated/process_security_environment_specification/README.md |
Documents regeneration. |
src/core/generated/process_security_environment_specification/regenerate.ps1 |
Adds binding generator script. |
src/core/generated/process_security_environment_specification/src/lib.rs |
Exports generated modules. |
…/destination_rule_generated.rs |
Defines destination-rule bindings. |
…/endpoint_policy_generated.rs |
Defines endpoint-policy bindings. |
…/endpoint_rule_generated.rs |
Defines endpoint-rule bindings. |
…/filter_action_generated.rs |
Defines filter-action bindings. |
…/ip_protocol_generated.rs |
Defines protocol bindings. |
…/ip_subnet_generated.rs |
Defines subnet bindings. |
…/network_policy_generated.rs |
Defines network-policy bindings. |
…/port_rule_generated.rs |
Defines port-rule bindings. |
…/process_security_environment_generated.rs |
Defines root PSEC bindings. |
…/proxy_info_generated.rs |
Defines proxy bindings. |
…/schema_version_generated.rs |
Defines schema-version bindings. |
Use a V2-specific denied-path capability error and document the complete process security-environment probe surface. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3a8860a9-bb20-48ef-91a1-3da8e34b92fb
Use the V2 non-consuming Stop contract for three bounded attempts on transient output contention while preserving permanent and exhausted HRESULT failures before exactly-once close. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3a8860a9-bb20-48ef-91a1-3da8e34b92fb
Gudge (MGudgin)
left a comment
Gudge (MGudgin)
left a comment
There was a problem hiding this comment.
Adversarial review — 13 axes
Ran a multi-axis adversarial review (security, reliability, performance, maintainability, correctness, testability, test coverage, documentation-drift, cross-platform-parity, concurrency, backward-compatibility, supply-chain, proportionality), each axis in an isolated context on a non-Claude model, then consolidated.
I then re-verified every finding that cited code outside the diff, to make sure this PR is not being blamed for pre-existing issues. That pass moved four findings out of scope and made one worse. Details below — I'd rather hand you a shorter, accurate list than a longer one.
What the review found clean, with receipts
The hardest parts of an ABI migration are right, and that's worth saying plainly:
- ABI fidelity — all six V2 exports match the documented contract, including the by-value
CloseProcessSecurityEnvironment(secenv.rs:78-79) and thevoid-returning infallibleCloseLearningModeTrace(ffi.rs:46). The pointer-vs-by-value trap is handled correctly. - HRESULT handling — everything goes through
HRESULT::is_err()(self.0 < 0), soS_FALSEis correctly treated as success — and there's an explicit regression test for it (successful_hresult_starts_retryable_trace,ffi.rs:381). - Security — System32-only DLL loading, exact+complete export matching, unpredictable 128-bit denial filenames with create-new semantics, and job-object/suspended-launch enforcement retained through the legacy removal.
- Concurrency — exact-once close, correct trace-before-environment drop order on success/error/unwind, handles nulled before
Drop, and the Ctrl-C handler shares no trace/PSEC state. - Proportionality — the hand-written churn maps to an externally forced ABI change; no scope creep found.
- Performance of the feature-off path — executions without
captureDenialspay zero; all new DLL loading, OS queries and PSEC construction are cleanly gated.
The one thing I'd fix first
CaptureSession::finish (lifecycle.rs:111) calls StopLearningModeTrace once, treats every HRESULT as terminal, and then closes the trace and environment unconditionally. Your ffi.rs layer implements the non-consuming/retryable contract correctly — failed_hresult_keeps_trace_live_until_close proves it — and then finish takes self by value and throws that capability away. A transient delivery failure fails an already-completed sandbox execution and permanently loses the denial document.
The verification pass — findings I am NOT attributing to this PR
These were raised by agents but I confirmed against git show 80cd261c:<path> that they are pre-existing. Flagging them only as context / possible follow-ups, not as things to fix here:
- Unchecked
ResumeThread(base_container_runner.rs:1614). Reported as a new hang risk. It is byte-identical between base and HEAD, and there is exactly oneCreateProcessWcall site in both revisions — so there is no new capture-specific launch. Pre-existing; the contrast with the AppContainer path (which does terminate and report) is a pre-existing inconsistency worth a separate issue. LearningModeErrorflattened intoio::Error::other(combine_capture_and_cleanup_results, HEAD:2014). Reported as a High testability blocker. The function is unchanged by this PR (base:1896); it arrived with #710. Your newLearningModeErrorvariants do flow through it and get stringified, so the diagnosability ceiling is real — but it isn't yours to fix in this PR.- The capture E2E is
#[ignore]and asserts vacuously (e2e_windows.rs:284).e2e_windows.rsis not modified by this PR and the test is identical at base — samecmd.exe /c echoworkload producing an emptydenialsarray. Pre-existing. The fair criticism is only that it wasn't extended to cover V2. --probedisagrees with the dispatcher. Reported as a High. The dispatcher already had acapture_denialsoverride at base (dispatcher.rs:342), andprobe.rsis unchanged, so the tier-misprediction divergence pre-dates this PR. However — see below, there is a genuinely new part.
Out-of-diff findings that ARE attributable
Medium — --probe doesn't reflect the new V2 export requirement. probe.rs is unchanged and still calls fallback_detector::detect directly (probe.rs:131), with no reference to capture_denials or Learning Mode anywhere in the file. The tier divergence is pre-existing, but this PR changes the availability threshold to the complete V2 export set, so --probe now reports success on a V1 host for a request that will fail backend_unavailable at validate(). Worth teaching run_probe about the V2 check and exposing it in ProbeFacts.
Medium — wire.rs / generated dev schema are now stale. src/core/wxc_common/src/wire.rs is unchanged, and I confirmed the captureDenials + leastPrivilege and captureDenials + network.proxy rejections (base_container_runner.rs:1679, :1686) have no equivalent at base — they are new here. Since wire.rs drives schemas/dev/, the generated schema and IDE hovers still describe the old contract. docs/schema.md got the note; its generator input didn't.
Medium — no Learning Mode row in docs/process-container/os-version-support.md. That file is unchanged. Given this PR establishes a hard OS build floor (validated V1 26657.1002 fails, V2 26663.1000 works), the per-release support matrix is where a user would look.
What I'd want before approving
- Retry Stop rather than discarding it (
lifecycle.rs:111) — bounded, transient-HRESULT-only. - Restore lifecycle test coverage — see the inline comment;
lifecycle.rswent from 2 tests to 0 while gaining the V2 state machine. - Add an FFI seam so export-set gating and the PSEC fail-closed paths can be tested without a V2 host.
- Update
wire.rsand regenerateschemas/dev/, and record the V2 build floor inos-version-support.md. - Record
.fbsprovenance and pinflatc— the repo already has the pattern in the WSLC bindings runbook.
The OnceLock memoisation, the workspace-member gating, and the spawn_base extraction all read as follow-ups. Nothing here suggests the migration is wrong — the implementation is in good shape; it's the verification that's thin relative to the blast radius. Happy to be pushed back on any of it, particularly the parity/build-cost point which is a judgement call.
microsoft-github-policy-service
Bot
added
the
Needs-Author-Feedback
Add host-independent lifecycle and fail-closed tests, memoize V2 capability discovery, document host requirements, regenerate schema surfaces, and pin/provenance-gate the generated PSEC contract. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3a8860a9-bb20-48ef-91a1-3da8e34b92fb
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
microsoft-github-policy-service
Bot
added
Needs-Attention
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 39 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/backends/learning_mode/windows/src/ffi.rs:411
- This public probe now bypasses the memoized loader and calls
LoadLibraryExWon every invocation. Becauseprobe_learning_mode_exportsintentionally never callsFreeLibrary, repeated availability checks keep incrementing the DLL reference count and contradict the PR's process-wide export-resolution memoization. The memoized loader already requires all three V2 exports, so use it here.
probe_learning_mode_exports().is_complete()
scripts/versioning/check-psec-codegen.js:252
- The regenerate-and-diff gate formats the temporary crate from the repository root, so rustup does not see
src/rust-toolchain.tomland instead uses the workflow's movingstabletoolchain. The documented regeneration script formats fromsrc/with pinned Rust 1.93; once rustfmt output changes, unchanged generated bindings can fail this byte comparison. Setcwdtosrcfor this invocation so both paths use the pinned formatter.
fs.writeFileSync(join(genCrate, "Cargo.toml"), FMT_MANIFEST);
execFileSync("cargo", ["fmt", "--manifest-path", join(genCrate, "Cargo.toml")], {
stdio: ["ignore", "ignore", "inherit"],
});
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 52e48fee-23e1-4b05-803c-522812fcdda2
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 39 changed files in this pull request and generated no new comments.
Suppressed comments (3)
src/backends/learning_mode/windows/src/lifecycle.rs:107
finish(None)now callsStopLearningModeTrace(including its retry policy), but the production early-failure paths still usefinish(None)to discard sessions (base_container_runner.rs:1522-1525,1627-1629, and1741-1743). Under the V2 contract documented here, early-exit discard is close-without-Stop, so those paths can now sleep and report a spurious teardown failure for an operation that discard does not require. Give callers an explicit infallible discard path (or drop the session) and reservefinish/Stop for ETL delivery.
/// Stop the trace and deliver it to `output_path` (or skip delivery when
/// `None`), retry transient delivery failures, close the trace, then close
/// the security environment. Call **after** the child has exited.
scripts/versioning/check-psec-codegen.js:252
- This regeneration runs
cargo fmtfrom the repository root against a manifest under the temp directory, so rustup does not discoversrc/rust-toolchain.tomland uses the runner's defaultstabletoolchain. The checked-in regeneration script formats fromsrcwith pinned Rust 1.93, meaning a rustfmt change in a newer stable release can make this byte-comparison gate fail despite correct generated output. Run this command withcwdset to the repository'ssrcdirectory so both paths honor the same pin.
execFileSync("cargo", ["fmt", "--manifest-path", join(genCrate, "Cargo.toml")], {
stdio: ["ignore", "ignore", "inherit"],
});
docs/schema.md:76
- The complete example declares schema
0.6.0-alphaat line 26, but the new runner validation rejectscaptureDenialsbefore schema 0.8. It also rejectsleastPrivilegeandnetwork.proxyfor every schema 0.8+ PSEC request, not only when capture is enabled. As written, this example is guaranteed to fail and the new comments understate the schema-wide restrictions; update the example/version structure so it presents a valid configuration and documents both constraints.
// captureDenials cannot be combined with leastPrivilege.
// captureDenials cannot currently be combined with network.proxy.
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
| // flatc --conform ProcessSecurityEnvironment.previous.fbs ProcessSecurityEnvironment.fbs | ||
| // | ||
| // --conform verifies that every field/enum/table in the old schema still exists | ||
| // at the same vtable slot, type, and default in the new schema. |
There was a problem hiding this comment.
This schema is copied from the official OS PSEC schema, including the FlatBuffer evolution guidance, so I retained it to keep the checked-in contract aligned with its source. If you intended a different specific section on line 32 to be removed, could you clarify which portion?
| } | ||
| } | ||
|
|
||
| impl SecurityEnvironmentApi { |
There was a problem hiding this comment.
it's the only place that uses it at the moment. I introduced it since it was needed to call Brian's API.
There was a problem hiding this comment.
Good question. secenv is now used by both ordinary PSEC execution and Learning Mode capture, so it is broader than Learning Mode. Moving it directly into appcontainer_common would create a dependency cycle because the Learning Mode lifecycle also consumes it; the clean separation would be a dedicated Windows process-security-environment support crate. I kept that structural extraction out of this ABI/fallback fix, but agree it should be tracked separately.
sergeyshilov does this work? Refers to: src/backends/appcontainer/common/src/base_container_runner.rs:2125 in b9368ff. [](commit_id = b9368ff, deletion_comment = False) |
Route proxy requests away from capability-aware SBOX contracts until MXC can author the required AppContainer peer identity. Add the official security-environment API-set probe and retain legacy SBOX proxy behavior on query-less hosts. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 52e48fee-23e1-4b05-803c-522812fcdda2
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 40 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/backends/appcontainer/common/src/dispatcher.rs:345
- This reintroduces the schema-0.8 legacy fallback that the PR contract explicitly excludes.
is_usable_for_requestcan return true via SBOX when PSEC is unavailable/incompatible, so ordinary 0.8 requests may run through SBOX (or AppContainer) rather than failingbackend_unavailable; it also makesdry_runwithcaptureDenialsdepend on host probes before runner validation. Select BaseContainer/PSEC directly for every schema 0.8+ request and let its validation report unsupported PSEC capabilities without entering legacy tier detection.
// Keep the established tier fallback behavior for every schema version.
// For schema 0.8+, BaseContainerRunner prefers PSEC when available and
// otherwise uses the transitional SBOX contract. If neither BaseContainer
// contract is usable, detection continues to the AppContainer tiers.
let prefer_base_container = BaseContainerRunner::is_usable_for_request(request);
|
Branden Bonaby (@bbonaby) FYI: PR #739 now includes a temporary MXC compatibility fallback for the SBOX model-2 proxy contract introduced by OS PR 16153076. The updated OS contract requires Validated on build |
|
Jeff Whiteside (@jsidewhite) On the |
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Branden Bonaby (bbonaby)
left a comment
Branden Bonaby (bbonaby)
left a comment
There was a problem hiding this comment.
We can follow up with Sergey about jeffs comment, and the refactoring.
There are multiple requests for additional work to be done in follow on PRs. We will need to track that work and ensure it is complete.
microsoft-github-policy-service
Bot
removed
the
Needs-Attention
The README said the vendored schema "is not publicly redistributable", which reads as a restriction on the schema itself. The restriction is on the internal OS tree it was taken from: the schema text carries an MIT header and has been committed to this public repository since microsoft#739. Add the repository field so the published crate carries a source link. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ab22cd6f-f3d9-4325-8397-9e998aa058c3
📖 Description
Updates
captureDenialsto the official V2 Windows Learning Mode and processsecurity-environment contracts.
HRESULT StartLearningModeTraceHRESULT StopLearningModeTraceCloseLearningModeTrace(sharing/lock contention, busy, or retry requested), while permanent errors
remain single-attempt and the final HRESULT is preserved.
HRESULT CreateProcessSecurityEnvironmentQueryProcessSecurityEnvironmentSupportCloseProcessSecurityEnvironmentCreateProcessW+PROC_THREAD_ATTRIBUTE_SECURITY_ENVIRONMENTlaunch path; no legacyCreateProcessAsUserInsideSecurityEnvironmentdependency remains.while ordinary requests retain SBOX/AppContainer fallback when PSEC is
unavailable or cannot represent the requested policy.
captureDenialsremains PSEC-only and fails closed.
IsApiSetImplemented("api-win-appmodel-processmodel~securityenvironment")before resolving the official security-environment exports.
leastPrivilege, proxy, and deny-policy combinations through compatiblelower tiers; capture and host-list combinations still fail closed.
backend_unavailableinstead of invoking an incompatible ABI.deny-support query.
PSEC fail-closed tests.
exact
flatcrelease assets/checksums) and adds a CI gate that regeneratesand byte-compares the bindings with the pinned compiler.
contract introduced by Emit captureDenials JSON output (replacement for #701) #710.
Proxy compatibility fallback
Windows OS PR 16153076 changed the capability-aware SBOX proxy contract. Proxy
requests now require an AppContainer-hosted proxy named through
allowed_appcontainer_peer, requireprivateNetworkClientServer, and rejectinternetClient. MXC does not yet author that model-2 peer identity.Until that integration is implemented, hosts exposing
Experimental_QuerySandboxSupportroute proxy requests to the AppContainercompatibility path instead of launching an invalid SBOX request. Query-less
hosts retain the legacy SBOX proxy behavior. This is intentionally a temporary
compatibility fallback; model-2 SBOX support should be implemented separately.
VM validation on build
26663.1000confirmed minimal built-in WinHTTP proxyE2Es pass for schema
0.7.0-alphaand0.8.0-alpha. The full Node integrationpolicy additionally grants SDK tool paths such as
C:\Debuggers; when thefallback reaches AppContainer+DACL, the test account must have
WRITE_DAConthose paths. That host permission is not required once MXC supports model-2 T1.
The separate OS-side ETW provider-readiness improvement is intentionally out of
scope for this ABI update.
🔗 References
🔍 Validation
cargo fmt --all -- --checkcargo test -p learning_mode_core -p learning_mode_windows -p appcontainer_common --all-targetscargo test -p wxc --no-runcargo test -p wxc_e2e_tests --no-runcargo clippy -p learning_mode_core -p learning_mode_windows -p appcontainer_common --all-targets -- -D warningscargo check --workspace --all-targetsnode scripts/versioning/check-schema-codegen.jsnode scripts/versioning/check-sdk-types-codegen.jsnode scripts/versioning/check-psec-codegen.jsnode scripts/versioning/validate-configs.jslifecycle/concurrency, data-validation, and error-handling reviews found no
blocking MXC issues.
26657.1002):captureDenialsreturnsFailurePhase::BackendUnavailable.functional through the legacy SBOX path.
26663.1000):metadata.
✅ Checklist
Cargo.lock, thedependency-feed-checkcheck passes (see docs/pull-requests.md)📋 Issue Type
GitHub Actions runs the PR validation build automatically. The ADO pipeline
(
MXC-PR-Build) is the Azure version of the PR pipeline, kept in parity withthe GitHub Actions build; it runs on merge to
main, and Microsoft reviewerswith write access can trigger it on a PR with
/azp run. Seedocs/pull-requests.md.
If the
dependency-feed-checkcheck fails on a new dependency, the crate mustbe added to the feed before the PR can pass. See
docs/pull-requests.md
for the steps.