feat(intune): model remediation detection and remediation evidence - #400
Merged
Conversation
Before: `intune::apps::windows::scripts::redaction` owned the UPN, user-profile and command-line masking regexes outright. A second Windows workload analyzer needs exactly the same masking, and copying those four regexes would create two implementations that drift apart -- the class of duplication that already drew review comment on #386. This moves the masking primitives to `intune::apps::windows::common`, unchanged. `scripts::redaction` now calls into it and keeps deciding *what* is sensitive, which is the part that genuinely differs per analyzer. `scripts::redact_text` stays exported from the same path it was, so no consumer moves. This is the right seam because the split is between two different kinds of decision. Which spans of free text are dangerous is a property of Windows log text in general. Which fields carry that text is a property of one analyzer's contract. Only the first is shared. Verified with `cargo test --locked -p cmtraceopen-parser`: 24 `intune_windows_scripts` integration tests pass unchanged, including the redaction and golden-export cases, plus 407 unit and 222 esp tests. 0 failed. `cargo clippy --locked -p cmtraceopen-parser --all-targets -- -D warnings` is clean. Refs #360 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Before: Intune Remediations had no owner. `intune/event_tracker.rs` recognised
a `HealthScripts` source kind and emitted loose events, but nothing modelled the
pair. There was no way to answer whether detection ran, whether remediation was
required, whether it ran, what it did, or whether any of it was reported.
This adds `cmtraceopen_parser::intune::apps::windows::remediations`, a semantic
analyzer over supplied IME evidence:
- `sources.rs` classifies each artifact from its name *and* the CCM components
inside it, and recognises retained `{policyId}_{runId}.output` / `.error`
artifacts by name without ever reading their contents;
- `rules.rs` classifies one record at a time and decides nothing;
- `reducer.rs` groups records into pairs keyed on policy, run, and invocation,
reducing detection and remediation into *separate* stage outcomes;
- `redaction.rs` masks record text, artifact paths, and embedded payloads.
The rule everything else rests on: **an exit code with no stage terminates
nothing.** `0` means "compliant" from a detection script and "succeeded" from a
remediation script. Attributing a code to the wrong half inverts the diagnosis,
so a record that does not name its stage yields no exit token at all, and an
`AgentExecutor.log` supplied without its `HealthScripts.log` orchestrator
produces no transactions rather than guessed ones. `exit_tokens_never_cross_stages`
asserts this across four scenarios, and `RemediationExitToken::stage` is not
optional, so an unattributed token cannot be constructed.
Invocation is part of the key, so a scheduled run and an on-demand run of the
same policy never pool. `Skipped` is a distinct remediation state from
`NotStarted`: it is the stronger claim that detection proved there was nothing
to do.
Embedded JSON payloads are preserved losslessly and marked parsed or not. A
malformed payload is reported as malformed, never repaired, and a payload whose
braces do not close inside one record is not captured at all -- fragments are
never concatenated across records to complete one.
One defect was found by these fixtures and fixed in the shared masking: a
Windows path inside a JSON payload arrives escaped as `C:\\Users\\Someone`, and
the profile-segment rule required a single separator, so every such path was
exported unmasked. Remediation payloads are the most likely place for that to
happen, since detection scripts emit arbitrary data.
Verified from the repository root:
- `cargo test --locked -p cmtraceopen-parser`: 434 unit (was 407), 22 new
`intune_windows_remediations` integration, 24 scripts, 27 company-portal,
222 esp, 1 doc test; 0 failed.
- `cargo clippy --locked -p cmtraceopen-parser --all-targets -- -D warnings`:
clean.
All 17 scenarios required by the issue are present under
`tests/fixtures/intune/windows/remediations/`, each with a manifest, its
evidence, and a written-out `expected.json`.
`detection-compliant-remediation-skipped` additionally carries
`expected-full.json`, a golden of the complete redacted export.
This deliberately does not use `Closes`. The issue also asks that the relevant
`HealthScripts`/`AgentExecutor` logic in `intune/event_tracker.rs` be
consolidated behind the canonical API. That is not done here, so two rulesets
still exist in the crate; the same gap is tracked on #359.
Refs #360
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 28 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (14)
📝 WalkthroughWalkthroughAdds a Windows Intune remediation analyzer. It classifies artifacts and records, reduces stage-specific evidence into transactions, applies deterministic privacy redaction, and validates outcomes with comprehensive fixtures and integration tests. ChangesIntune Windows remediation analysis
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant SourceInput
participant ArtifactClassifier
participant RecordClassifier
participant BundleReducer
participant RedactionProjection
SourceInput->>ArtifactClassifier: classify artifact metadata
ArtifactClassifier->>RecordClassifier: provide scoped artifact content
RecordClassifier->>BundleReducer: provide record classifications
BundleReducer->>RedactionProjection: provide RemediationAnalysis
RedactionProjection-->>SourceInput: return redacted analysis
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
Comment |
coderabbitai
Bot
added
enhancement
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a new pure semantic analyzer under cmtraceopen_parser::intune::apps::windows::remediations to model the paired Intune Remediations lifecycle (detection + remediation) from supplied IME evidence, with strict stage-scoped exit semantics and deterministic redaction. It also factors shared text-masking into intune::apps::windows::common so the platform-scripts and remediations analyzers reuse one implementation.
Changes:
- Introduces
intune::apps::windows::remediations(source classification, record rules, reducer, models, redaction) plus an integration test and a full fixture matrix. - Extracts shared deterministic redaction into
intune::apps::windows::common::redact_textand re-exports it from the scripts module to preserve the public API. - Adds golden/contract tests (including “exit tokens never cross stages” and redaction idempotence) and scenario fixtures with explicit
expected.jsonoutputs.
Reviewed changes
Copilot reviewed 47 out of 80 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/cmtraceopen-parser/src/intune/apps/windows/mod.rs | Exposes new common and remediations analyzers under Windows Intune workloads. |
| crates/cmtraceopen-parser/src/intune/apps/windows/common/mod.rs | New shared module for common primitives (currently redaction). |
| crates/cmtraceopen-parser/src/intune/apps/windows/common/redaction.rs | Shared deterministic text masking (UPN/user-path/command-line), incl. JSON-escaped path fix + tests. |
| crates/cmtraceopen-parser/src/intune/apps/windows/scripts/mod.rs | Re-exports shared redact_text; keeps scripts API stable while moving implementation. |
| crates/cmtraceopen-parser/src/intune/apps/windows/scripts/redaction.rs | Updates scripts export projection to use shared common::redact_text. |
| crates/cmtraceopen-parser/src/intune/apps/windows/remediations/mod.rs | New remediation analyzer module surface + exports. |
| crates/cmtraceopen-parser/src/intune/apps/windows/remediations/models.rs | New public camelCase-serialized remediation contract types. |
| crates/cmtraceopen-parser/src/intune/apps/windows/remediations/sources.rs | Artifact classification for remediation evidence (name + component confirmation). |
| crates/cmtraceopen-parser/src/intune/apps/windows/remediations/rules.rs | Record-level classification rules (stage, keys, exit tokens, payload capture). |
| crates/cmtraceopen-parser/src/intune/apps/windows/remediations/reducer.rs | Reducer that pairs detection/remediation by key and produces stage outcomes + coverage. |
| crates/cmtraceopen-parser/src/intune/apps/windows/remediations/redaction.rs | Remediations export projection applying shared redaction to sensitive text/payloads. |
| crates/cmtraceopen-parser/tests/intune_windows_remediations.rs | New focused integration test over the fixture matrix + golden redacted export. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/detection-compliant-remediation-skipped/manifest.json | Fixture manifest for “detection compliant / remediation skipped”. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/detection-compliant-remediation-skipped/HealthScripts.log | Fixture evidence: HealthScripts (compliant). |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/detection-compliant-remediation-skipped/AgentExecutor.log | Fixture evidence: AgentExecutor (present but unkeyed). |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/detection-compliant-remediation-skipped/expected.json | Expected reduced contract for scenario. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/detection-compliant-remediation-skipped/expected-full.json | Golden full redacted export for scenario. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/remediation-succeeds-post-state-compliant/manifest.json | Fixture manifest for “remediation succeeds; post-state compliant”. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/remediation-succeeds-post-state-compliant/HealthScripts.log | Fixture evidence: HealthScripts (noncompliant → remediation → compliant + report). |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/remediation-succeeds-post-state-compliant/AgentExecutor.log | Fixture evidence: AgentExecutor. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/remediation-succeeds-post-state-compliant/expected.json | Expected reduced contract for scenario. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/remediation-exits-nonzero/manifest.json | Fixture manifest for “remediation exits nonzero”. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/remediation-exits-nonzero/HealthScripts.log | Fixture evidence: HealthScripts (remediation exit 3). |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/remediation-exits-nonzero/AgentExecutor.log | Fixture evidence: AgentExecutor. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/remediation-exits-nonzero/expected.json | Expected reduced contract for scenario. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/detection-launch-failure/manifest.json | Fixture manifest for “detection launch failure”. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/detection-launch-failure/HealthScripts.log | Fixture evidence: HealthScripts (failed to create process). |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/detection-launch-failure/AgentExecutor.log | Fixture evidence: AgentExecutor. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/detection-launch-failure/expected.json | Expected reduced contract for scenario. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/detection-timeout/manifest.json | Fixture manifest for “detection timeout”. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/detection-timeout/HealthScripts.log | Fixture evidence: HealthScripts (timed out). |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/detection-timeout/AgentExecutor.log | Fixture evidence: AgentExecutor. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/detection-timeout/expected.json | Expected reduced contract for scenario. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/remediation-launch-failure/manifest.json | Fixture manifest for “remediation launch failure”. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/remediation-launch-failure/HealthScripts.log | Fixture evidence: HealthScripts (remediation failed to create process). |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/remediation-launch-failure/AgentExecutor.log | Fixture evidence: AgentExecutor. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/remediation-launch-failure/expected.json | Expected reduced contract for scenario. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/remediation-timeout/manifest.json | Fixture manifest for “remediation timeout”. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/remediation-timeout/HealthScripts.log | Fixture evidence: HealthScripts (remediation timed out). |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/remediation-timeout/AgentExecutor.log | Fixture evidence: AgentExecutor. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/remediation-timeout/expected.json | Expected reduced contract for scenario. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/remediation-succeeds-post-state-noncompliant/manifest.json | Fixture manifest for “remediation succeeds; post-state noncompliant”. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/remediation-succeeds-post-state-noncompliant/HealthScripts.log | Fixture evidence: HealthScripts (post-detection noncompliant). |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/remediation-succeeds-post-state-noncompliant/AgentExecutor.log | Fixture evidence: AgentExecutor. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/remediation-succeeds-post-state-noncompliant/expected.json | Expected reduced contract for scenario. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/execution-completes-report-fails/manifest.json | Fixture manifest for “report fails”. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/execution-completes-report-fails/HealthScripts.log | Fixture evidence: HealthScripts (send failed). |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/execution-completes-report-fails/AgentExecutor.log | Fixture evidence: AgentExecutor. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/execution-completes-report-fails/expected.json | Expected reduced contract for scenario. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/scheduled-and-on-demand-kept-separate/manifest.json | Fixture manifest for “scheduled vs on-demand separation”. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/scheduled-and-on-demand-kept-separate/HealthScripts.log | Fixture evidence: HealthScripts (two runs). |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/scheduled-and-on-demand-kept-separate/AgentExecutor.log | Fixture evidence: AgentExecutor. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/scheduled-and-on-demand-kept-separate/expected.json | Expected reduced contract for scenario. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/retry-sequence/manifest.json | Fixture manifest for “retry sequence”. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/retry-sequence/HealthScripts.log | Fixture evidence: HealthScripts (remediation attempts). |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/retry-sequence/AgentExecutor.log | Fixture evidence: AgentExecutor. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/retry-sequence/expected.json | Expected reduced contract for scenario. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/missing-agent-executor/manifest.json | Fixture manifest for “missing AgentExecutor is coverage”. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/missing-agent-executor/HealthScripts.log | Fixture evidence: HealthScripts only. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/missing-agent-executor/expected.json | Expected reduced contract for scenario. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/missing-health-scripts/manifest.json | Fixture manifest for “missing HealthScripts stages nothing”. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/missing-health-scripts/AgentExecutor.log | Fixture evidence: AgentExecutor only. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/missing-health-scripts/expected.json | Expected reduced contract for scenario. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/malformed-embedded-payload/manifest.json | Fixture manifest for “malformed embedded JSON”. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/malformed-embedded-payload/HealthScripts.log | Fixture evidence: HealthScripts with malformed payload. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/malformed-embedded-payload/AgentExecutor.log | Fixture evidence: AgentExecutor. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/malformed-embedded-payload/expected.json | Expected reduced contract for scenario. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/rotation-boundary/manifest.json | Fixture manifest for rotation boundary behavior. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/rotation-boundary/HealthScripts-1.log | Fixture evidence: rotated HealthScripts part 1. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/rotation-boundary/HealthScripts.log | Fixture evidence: live HealthScripts tail. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/rotation-boundary/AgentExecutor.log | Fixture evidence: AgentExecutor. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/rotation-boundary/expected.json | Expected reduced contract for scenario. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/same-minute-distinct-policies/manifest.json | Fixture manifest for “same-minute distinct policies”. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/same-minute-distinct-policies/HealthScripts.log | Fixture evidence: HealthScripts (two policies, same timestamp). |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/same-minute-distinct-policies/AgentExecutor.log | Fixture evidence: AgentExecutor. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/same-minute-distinct-policies/expected.json | Expected reduced contract for scenario. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/privacy-redaction/manifest.json | Fixture manifest for deterministic privacy redaction. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/privacy-redaction/HealthScripts.log | Fixture evidence: sensitive values + embedded JSON. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/privacy-redaction/AgentExecutor.log | Fixture evidence: sensitive command-line flag value. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/privacy-redaction/expected.json | Expected reduced contract + redaction assertions. |
…alyzer 1. A block never closed, so a stray record on a reused thread could overwrite a terminal outcome. Blocks only split at a stage launch, and nothing ended one when a run reported. The agent reuses CCM thread numbers freely, so a later record that named no policy of its own was absorbed into whatever block was still open, inherited that run's key and stage, and its exit code was applied to a transaction it had nothing to do with -- silently inverting a diagnosis with no conflict raised, because the record never stated a policy to conflict with. Blocks now also open at a policy receipt and close after a report. Fixture `thread-reuse-after-report` pins it. 2. A comment promised a safety net that did not exist. It claimed that "two policies, or two stages" in one block would refuse to key, but only the policy conflict returned early. The behaviour on a stage conflict is intentional -- the records share a policy, so the key is safe; the stage is not, so it is withheld and those records stay unstaged -- but the comment described something else entirely. Corrected to say what the code does and why the two conflicts are handled differently. 3. Two different constants named `IME_COMPONENTS` lived in the same module with different contents: one gating which records may speak for this workload, one confirming that a file is the IME log at all. Renamed to `IME_RECORD_SCOPE_COMPONENTS` and `IME_FILE_COMPONENTS`, since the wider file list looked like a bug next to the narrower record list. 4. The module's most subtle rule was undocumented and read as an accident: a record may inherit its block's stage for *routing* but never for *interpreting* an exit code. Spelled out in the module docs with the reason -- routing a record to the wrong stage costs an unresolved half, while interpreting a code under the wrong stage turns "compliant" into "succeeded" -- and pinned with a test. Reviewed and rejected: that `PowerShell` be accepted as a remediation-scope component in the primary IME log, on the grounds that the sibling script analyzer accepts it there. It is exactly because the sibling owns `PowerShell` in that log that this module must not: accepting it would make platform-script reporting records become remediation reporting records, which is the cross-workload contamination the scope gate exists to prevent. Remediation orchestration is logged under `HealthScripts`. Two tests now pin both directions, and the reasoning is in the constant's documentation. Verified from the repository root: - `cargo test --locked -p cmtraceopen-parser`: 437 unit, 23 `intune_windows_remediations`, 24 scripts, 27 company-portal, 222 esp, 1 doc test; 0 failed. - `cargo clippy --locked -p cmtraceopen-parser --all-targets -- -D warnings`: clean. - `git diff --check`: clean. Refs #360 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (6)
crates/cmtraceopen-parser/tests/intune_windows_remediations.rs (1)
319-345: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider widening
exit_tokens_never_cross_stagescoverage.The doc comment on Line 316 calls this the property "the whole module rests on," but the loop on Line 321-326 checks only 4 scenarios. Other scenarios in this fixture set, such as
same-minute-distinct-policiesandscheduled-and-on-demand-kept-separate, also carry a non-nulldetectionExitDecimaland would exercise the detection-side half of this invariant at negligible cost. Add these scenario names to the list to strengthen regression coverage of a core invariant.♻️ Suggested addition
for scenario in [ "remediation-succeeds-post-state-compliant", "remediation-exits-nonzero", "remediation-succeeds-post-state-noncompliant", "retry-sequence", + "same-minute-distinct-policies", + "scheduled-and-on-demand-kept-separate", ] {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/cmtraceopen-parser/tests/intune_windows_remediations.rs` around lines 319 - 345, Expand the scenario list in exit_tokens_never_cross_stages to include same-minute-distinct-policies and scheduled-and-on-demand-kept-separate, ensuring the detection-stage invariant is exercised for these fixtures while preserving the existing assertions and scenarios.crates/cmtraceopen-parser/src/intune/apps/windows/remediations/reducer.rs (3)
506-520: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the sort-key closure so it does not shadow the
keyparameter.The closure at Line 508 is named
key, and it shadows thekey: RemediationKeyparameter for the body of the sort closure. The code compiles, because the parameter is not read inside. A later edit that needs the transaction key there would silently pick up the closure instead.♻️ Proposed rename
if all_trustworthy { ordered.sort_by(|&left, &right| { - let key = |index: usize| { + let sort_key = |index: usize| { ( records[index] .timestamp .as_ref() .and_then(|t| t.normalized_utc.clone()) .unwrap_or_default(), records[index].artifact_index, records[index].record_number, ) }; - key(left).cmp(&key(right)) + sort_key(left).cmp(&sort_key(right)) });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/cmtraceopen-parser/src/intune/apps/windows/remediations/reducer.rs` around lines 506 - 520, Rename the inner sort-key closure in the all_trustworthy sorting block to avoid shadowing the surrounding key: RemediationKey parameter, and update its call sites in the comparator. Keep the sorting behavior unchanged.
142-159: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign the comment with the two distinct conflict behaviors.
The comment states that a policy conflict or a stage conflict makes the code refuse to key. The code refuses to key only on
conflicting_policy. Onconflicting_stageit still assignsresolved_policy_idandresolved_run_id, and only withholdsresolved_stage.The code behavior is the more useful one, because an unstaged but keyed record still contributes reporting and coverage evidence. Correct the comment so the contract is unambiguous.
♻️ Proposed comment correction
- // Two policies, or two stages, inside one block means our block boundary is - // wrong for this agent version. Refuse to key rather than merge. + // Two policies inside one block means our block boundary is wrong for this + // agent version. Refuse to key rather than merge. if conflicting_policy { return; } for &index in block { if let Some(policy_id) = &policy_id { records[index].resolved_policy_id = Some(policy_id.clone()); records[index].resolved_run_id = run_id.clone(); records[index].resolved_invocation = invocation; } + // Two stages inside one block are also a boundary error, but the key + // still holds. Withhold only the stage, so no exit code is attributed. if !conflicting_stage {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/cmtraceopen-parser/src/intune/apps/windows/remediations/reducer.rs` around lines 142 - 159, Update the comment above the conflicting_policy check to distinguish the two behaviors: conflicting policies prevent keying the block, while conflicting stages only prevent assigning a resolved stage and still allow policy and run identifiers to be assigned. Keep the existing control flow unchanged.
357-357: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider limiting observations to in-scope records.
observationsreceives one entry per parsed line of every non-output artifact, and each entry clones the full message text at Line 230. Out-of-scope records are included, becauseanalyze_remediation_bundlepushes aPendingRecordfor every line at Lines 293-307 regardless ofclassification.in_scope.
IntuneManagementExtension.logis shared by every workload, so most of its lines are out of scope for remediations. For a large rotated bundle this holds two copies of the whole log in memory, and it enlarges the exported analysis with records that carrysignal: Unclassifiedand no key.Filtering
to_observationto records whereclassification.in_scopeis true, or where the record is keyed or carries a signal, would bound the output. Confirm the fixtures do not depend on the current full-fidelity list before changing it.♻️ Proposed filter
- let observations = records.iter().map(to_observation).collect(); + let observations = records + .iter() + .filter(|record| record.classification.in_scope) + .map(to_observation) + .collect();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/cmtraceopen-parser/src/intune/apps/windows/remediations/reducer.rs` at line 357, Limit the observations collected in the reducer around `records.iter().map(to_observation)` to records relevant to remediation analysis, using `classification.in_scope` or the presence of a key or signal, while preserving all currently included in-scope records. Review and update affected fixtures or expectations so they no longer depend on out-of-scope unclassified entries.crates/cmtraceopen-parser/src/intune/apps/windows/remediations/redaction.rs (2)
44-49: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueDocument that
parseddescribes the pre-redaction text.
redact_payloadmasksraw_textand copiesparsedunchanged. Masking can replace characters inside the JSON, so an exported payload can carryparsed: truenext to text that no longer parses as JSON.Keeping the original verdict is correct, because
parsedis evidence about what the agent emitted. A consumer that re-parsesraw_textand compares the result againstparsedwould reach the wrong conclusion. State the contract on theparsedfield inmodels.rs, or in this function.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/cmtraceopen-parser/src/intune/apps/windows/remediations/redaction.rs` around lines 44 - 49, Document in the RemediationPayload model or redact_payload function that parsed records whether the original, pre-redaction raw_text was valid JSON and is intentionally preserved unchanged after redaction. Keep the existing parsed value while clarifying that it must not be interpreted as the parse status of the redacted text.
30-56: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winDestructure exhaustively so a new sensitive field cannot escape masking.
All four helpers copy the remaining fields with
..value.clone(). If a later change adds aRemediationClassifiedStringfield toRemediationArtifact,RemediationObservation,RemediationPayload, orRemediationTransaction, the compiler accepts these helpers unchanged and the new field is exported unmasked.This module is the privacy boundary for the export, so a compile-time failure is preferable to a silent leak. Replace the functional update with an exhaustive struct pattern in each helper. The compiler then rejects the helper until the new field is classified.
The golden test at
crates/cmtraceopen-parser/tests/intune_windows_remediations.rsdetects this only if a fixture happens to carry sensitive content in the new field.♻️ Proposed pattern for one helper
fn redact_payload(payload: &RemediationPayload) -> RemediationPayload { + // Destructured, not spread: a new field must be classified here before + // this compiles. + let RemediationPayload { + evidence, + parsed, + raw_text, + } = payload; RemediationPayload { - raw_text: redact_classified(&payload.raw_text), - ..payload.clone() + evidence: evidence.clone(), + parsed: *parsed, + raw_text: redact_classified(raw_text), } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/cmtraceopen-parser/src/intune/apps/windows/remediations/redaction.rs` around lines 30 - 56, Replace the functional updates using `..artifact.clone()`, `..observation.clone()`, `..payload.clone()`, and `..transaction.clone()` in `redact_artifact`, `redact_observation`, `redact_payload`, and `redact_transaction` with exhaustive struct destructuring and reconstruction. Preserve existing field values and redaction behavior while explicitly handling every field, so adding a new classified field causes a compile-time failure until it is masked.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/cmtraceopen-parser/src/intune/apps/windows/common/redaction.rs`:
- Around line 69-77: Update command_line_re in the flag-to-value separator
pattern to accept whitespace, colon, or equals separators, ensuring values in
forms such as -Password:hunter2 and -Token=abc are redacted. Add regression
coverage through redact_text for both colon- and equals-separated credential
values while preserving existing whitespace-separated behavior.
In `@crates/cmtraceopen-parser/src/intune/apps/windows/remediations/reducer.rs`:
- Around line 401-422: Update the DetectionState::Failed |
DetectionState::TimedOut handling in the evidence-request match to terminate
explicitly regardless of has_output_evidence. For failed or timed-out detection,
inspect report and request the appropriate reporting evidence only when
reporting is not observed; otherwise return None, preventing fallthrough to the
remediation-stage match.
- Around line 629-643: Update apply_detection at
crates/cmtraceopen-parser/src/intune/apps/windows/remediations/reducer.rs:629-643
to preserve the existing state and exit_token when a StageCompleted record has
no readable code and the current state is Compliant or Noncompliant; otherwise
retain the current update behavior. Apply the same guard in apply_remediation at
crates/cmtraceopen-parser/src/intune/apps/windows/remediations/reducer.rs:655-666
for Succeeded or ExitedNonZero, preserving the determined state and token when
the new completion lacks a readable code.
In `@crates/cmtraceopen-parser/src/intune/apps/windows/remediations/rules.rs`:
- Around line 72-81: Update extract_ids or a sibling script-path helper to
return the optional stage captured by script_path_re, and update extract_stage
to prefer that path-derived detect/remediate value before falling back to
message wording. Propagate the stage through classify_record and extend
script_path_yields_both_key_halves to verify the captured stage, preserving
existing ID extraction and fallback behavior.
- Around line 335-357: Update extract_payload to track whether the scan is
inside a JSON string and whether the current character is escaped, ignoring
braces while inside string literals. Preserve the existing depth-based
extraction and parsed flag for non-string braces, and add coverage for a payload
containing a closing brace inside a string.
- Around line 253-275: Update the hex-sourced branch of the exit-code parsing
logic to derive hex_text from the parsed signed value, matching the decimal
branch’s normalized width and sign representation. Only produce hex_text when
parsing succeeds; unreadable or oversized literals must leave it as None. Reuse
the existing signed-value-to-hex conversion used by the decimal branch rather
than preserving the original source digits.
In `@crates/cmtraceopen-parser/src/intune/apps/windows/remediations/sources.rs`:
- Around line 85-97: Case-sensitive suffix checks misclassify mixed-case Windows
artifacts. In
crates/cmtraceopen-parser/src/intune/apps/windows/remediations/sources.rs lines
85-97, update output_artifact_key to use a shared case-insensitive
suffix-stripping helper for .output and .error; in lines 32-35, update the
log-artifact parsing path to use the same helper for .log. Define the helper
near split_rotation and add coverage for a mixed-case extension.
---
Nitpick comments:
In `@crates/cmtraceopen-parser/src/intune/apps/windows/remediations/redaction.rs`:
- Around line 44-49: Document in the RemediationPayload model or redact_payload
function that parsed records whether the original, pre-redaction raw_text was
valid JSON and is intentionally preserved unchanged after redaction. Keep the
existing parsed value while clarifying that it must not be interpreted as the
parse status of the redacted text.
- Around line 30-56: Replace the functional updates using `..artifact.clone()`,
`..observation.clone()`, `..payload.clone()`, and `..transaction.clone()` in
`redact_artifact`, `redact_observation`, `redact_payload`, and
`redact_transaction` with exhaustive struct destructuring and reconstruction.
Preserve existing field values and redaction behavior while explicitly handling
every field, so adding a new classified field causes a compile-time failure
until it is masked.
In `@crates/cmtraceopen-parser/src/intune/apps/windows/remediations/reducer.rs`:
- Around line 506-520: Rename the inner sort-key closure in the all_trustworthy
sorting block to avoid shadowing the surrounding key: RemediationKey parameter,
and update its call sites in the comparator. Keep the sorting behavior
unchanged.
- Around line 142-159: Update the comment above the conflicting_policy check to
distinguish the two behaviors: conflicting policies prevent keying the block,
while conflicting stages only prevent assigning a resolved stage and still allow
policy and run identifiers to be assigned. Keep the existing control flow
unchanged.
- Line 357: Limit the observations collected in the reducer around
`records.iter().map(to_observation)` to records relevant to remediation
analysis, using `classification.in_scope` or the presence of a key or signal,
while preserving all currently included in-scope records. Review and update
affected fixtures or expectations so they no longer depend on out-of-scope
unclassified entries.
In `@crates/cmtraceopen-parser/tests/intune_windows_remediations.rs`:
- Around line 319-345: Expand the scenario list in
exit_tokens_never_cross_stages to include same-minute-distinct-policies and
scheduled-and-on-demand-kept-separate, ensuring the detection-stage invariant is
exercised for these fixtures while preserving the existing assertions and
scenarios.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3c459974-445e-4094-88a3-d103551e980d
⛔ Files ignored due to path filters (33)
crates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/detection-compliant-remediation-skipped/AgentExecutor.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/detection-compliant-remediation-skipped/HealthScripts.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/detection-launch-failure/AgentExecutor.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/detection-launch-failure/HealthScripts.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/detection-timeout/AgentExecutor.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/detection-timeout/HealthScripts.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/execution-completes-report-fails/AgentExecutor.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/execution-completes-report-fails/HealthScripts.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/malformed-embedded-payload/AgentExecutor.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/malformed-embedded-payload/HealthScripts.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/missing-agent-executor/HealthScripts.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/missing-health-scripts/AgentExecutor.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/privacy-redaction/AgentExecutor.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/privacy-redaction/HealthScripts.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/remediation-exits-nonzero/AgentExecutor.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/remediation-exits-nonzero/HealthScripts.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/remediation-launch-failure/AgentExecutor.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/remediation-launch-failure/HealthScripts.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/remediation-succeeds-post-state-compliant/AgentExecutor.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/remediation-succeeds-post-state-compliant/HealthScripts.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/remediation-succeeds-post-state-noncompliant/AgentExecutor.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/remediation-succeeds-post-state-noncompliant/HealthScripts.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/remediation-timeout/AgentExecutor.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/remediation-timeout/HealthScripts.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/retry-sequence/AgentExecutor.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/retry-sequence/HealthScripts.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/rotation-boundary/AgentExecutor.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/rotation-boundary/HealthScripts-1.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/rotation-boundary/HealthScripts.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/same-minute-distinct-policies/AgentExecutor.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/same-minute-distinct-policies/HealthScripts.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/scheduled-and-on-demand-kept-separate/AgentExecutor.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/scheduled-and-on-demand-kept-separate/HealthScripts.logis excluded by!**/*.log
📒 Files selected for processing (47)
crates/cmtraceopen-parser/src/intune/apps/windows/common/mod.rscrates/cmtraceopen-parser/src/intune/apps/windows/common/redaction.rscrates/cmtraceopen-parser/src/intune/apps/windows/mod.rscrates/cmtraceopen-parser/src/intune/apps/windows/remediations/mod.rscrates/cmtraceopen-parser/src/intune/apps/windows/remediations/models.rscrates/cmtraceopen-parser/src/intune/apps/windows/remediations/redaction.rscrates/cmtraceopen-parser/src/intune/apps/windows/remediations/reducer.rscrates/cmtraceopen-parser/src/intune/apps/windows/remediations/rules.rscrates/cmtraceopen-parser/src/intune/apps/windows/remediations/sources.rscrates/cmtraceopen-parser/src/intune/apps/windows/scripts/mod.rscrates/cmtraceopen-parser/src/intune/apps/windows/scripts/redaction.rscrates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/detection-compliant-remediation-skipped/expected-full.jsoncrates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/detection-compliant-remediation-skipped/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/detection-compliant-remediation-skipped/manifest.jsoncrates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/detection-launch-failure/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/detection-launch-failure/manifest.jsoncrates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/detection-timeout/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/detection-timeout/manifest.jsoncrates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/execution-completes-report-fails/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/execution-completes-report-fails/manifest.jsoncrates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/malformed-embedded-payload/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/malformed-embedded-payload/manifest.jsoncrates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/missing-agent-executor/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/missing-agent-executor/manifest.jsoncrates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/missing-health-scripts/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/missing-health-scripts/manifest.jsoncrates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/privacy-redaction/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/privacy-redaction/manifest.jsoncrates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/remediation-exits-nonzero/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/remediation-exits-nonzero/manifest.jsoncrates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/remediation-launch-failure/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/remediation-launch-failure/manifest.jsoncrates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/remediation-succeeds-post-state-compliant/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/remediation-succeeds-post-state-compliant/manifest.jsoncrates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/remediation-succeeds-post-state-noncompliant/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/remediation-succeeds-post-state-noncompliant/manifest.jsoncrates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/remediation-timeout/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/remediation-timeout/manifest.jsoncrates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/retry-sequence/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/retry-sequence/manifest.jsoncrates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/rotation-boundary/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/rotation-boundary/manifest.jsoncrates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/same-minute-distinct-policies/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/same-minute-distinct-policies/manifest.jsoncrates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/scheduled-and-on-demand-kept-separate/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/intune/windows/remediations/scheduled-and-on-demand-kept-separate/manifest.jsoncrates/cmtraceopen-parser/tests/intune_windows_remediations.rs
All nine findings from Copilot and CodeRabbit were valid.
1. An IME log whose remediation records all carry `component="HealthScripts"`
was classified `Unknown` and never reached the reducer, because the
file-confirmation list omitted the one component this workload actually
uses there. Added.
2. A record naming an output path was treated as proof that the output artifact
was supplied. It is not: it proves the script wrote one somewhere on the
device. Using it to suppress the evidence request hid genuinely missing
artifacts. Only an artifact actually present in the bundle counts now.
3. Credential values separated by `:` or `=` were exported verbatim.
`-Password:hunter2` and `-Token=abc` are as common as the spaced form.
4. A detection that failed or timed out *with* output evidence fell through to
the remediation branch and asked for remediation records -- contradicting the
confidence rule in the same file, which treats a terminal detection as a
complete story where remediation is legitimately absent. A terminal
detection now always ends the request chain.
5. A later completion with no readable exit code erased a determined result.
Both stage appliers overwrote state unconditionally and mapped a missing
code to `InsufficientEvidence`, so a trailing generic "Detection script
finished" discarded what a readable code had already proved. Such a record
now contributes evidence without downgrading the state.
6. `script_path_re` captured `detect|remediate` and the captured group was
thrown away. A path such as `...\{policy}_{run}\detect.ps1` therefore named
no stage, because the prose rules require the word `script` to follow. The
path states the stage as plainly as the prose does; it is now used.
7. `hex_text` was copied from the source text rather than rendered from the
parsed value, so the same exit code rendered two ways (`0x1` against
`0x00000001`), a negative hex code lost its sign, and an unreadable
oversized literal still produced a hex form while `decimal` was `None`.
8. The payload brace scan counted braces inside JSON string literals, so
`{"Detail":"a } b","Compliant":false}` was truncated to `{"Detail":"a }` and
then reported as malformed -- losing the evidence the module exists to keep
and misreporting coverage. String and escape state are now tracked.
9. Extension matching enumerated fixed casings, so `HealthScripts.Log` and
`{policy}_{run}.Output` classified as `Unknown`. Windows file names are
case-insensitive; one shared helper now handles both sites.
Findings 7 and 8 are the two most likely to bite in practice, because detection
scripts emit arbitrary text into payload fields and vendors write exit codes in
either base.
Three fixtures were added for the behaviour changes that no existing scenario
covered: `detection-timeout-with-retained-artifact` (finding 4),
`later-completion-without-readable-code` (finding 5), and the case-insensitive
and IME-confirmation cases as unit tests (findings 1 and 9).
Verified from the repository root:
- `cargo test --locked -p cmtraceopen-parser`: 446 unit, 25
`intune_windows_remediations`, 24 scripts, 27 company-portal, 222 esp, 1 doc
test; 0 failed.
- `cargo clippy --locked -p cmtraceopen-parser --all-targets -- -D warnings`:
clean.
- `git diff --check`: clean.
Note for follow-up: finding 7 describes the same `hex_text` derivation that
exists in the already-merged `apps::windows::scripts::rules`. That module is not
touched here; it should get the same fix.
Refs #360
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 54 out of 90 changed files in this pull request and generated no new comments.
Suppressed comments (1)
crates/cmtraceopen-parser/src/intune/apps/windows/remediations/rules.rs:368
extract_payloadcounts{/}bytes without accounting for JSON strings/escapes. This will incorrectly terminate payload capture when a string value contains{or}(valid JSON), causing well-formed embedded payloads to be truncated and reported as malformed (or missed). Consider trackingin_string/escapeso brace depth only changes outside quoted strings.
.and_then(|caps| caps.name("run").map(|m| m.as_str().to_ascii_lowercase()));
(policy, run)
}
/// Extract a balanced top-level `{...}` span and report whether it parsed.
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
`main` gained the parser-family skeleton for epic #356 (#388), which reserved a slot at `intune::apps::windows::remediations` with a doc-only placeholder and declared the sibling `microsoft_store` and `win32` slots alongside it. Two conflicts, both in module declarations: - `apps/windows/mod.rs`: took main's list of reserved slots and added `common`, the shared masking module this branch introduces. - `remediations/mod.rs`: took this branch's implementation. The placeholder it replaces says "Implementation pending ... Add source classification, reduction, and findings submodules here", which is what this branch does. Not done in this merge, and worth stating plainly: #388 also landed `intune::evidence`, a shared contract carrying `IntuneTimestamp`, `IntuneEvidenceRef`, `IntuneSensitivity`, `IntuneFindingConfidence` and the observation-context envelope, and the placeholder asks leaves to consume it. This module defines its own `RemediationTimestamp`, `RemediationEvidenceRef`, `RemediationSensitivity` and `RemediationConfidence` instead, because it was written before that contract existed. Adopting it is the right end state but is deliberately not attempted here. The already-merged `apps::windows::scripts` leaf is in exactly the same position for the same reason, and migrating one sibling without the other would replace a shared gap with an inconsistency between two leaves that a reader would then have to reconcile. Both should move together, in a change whose diff is the migration and nothing else. Verified on the merged tree from the repository root: - `cargo test --locked -p cmtraceopen-parser`: 453 unit, 25 `intune_windows_remediations`, 24 scripts, 26 parser-family, 27 company-portal, 222 esp, 1 doc test; 0 failed. - `cargo clippy --locked -p cmtraceopen-parser --all-targets -- -D warnings`: clean. Refs #360 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
This was referenced Aug 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refs #360. Deliberately not
Closes— see "What is not done".What this adds
cmtraceopen_parser::intune::apps::windows::remediations— a semantic analyzer over supplied IME evidence for the Intune Remediations lifecycle. A remediation is a pair: detection decides whether anything is wrong, remediation runs only if it was, and the two halves are reduced separately.sources.rsrules.rsreducer.rsredaction.rsmodels.rsThe rule everything rests on
An exit code with no stage terminates nothing.
0means compliant from a detection script and succeeded from a remediation script. Attributing a code to the wrong half inverts the diagnosis.So: a record that does not name its stage yields no exit token at all,
RemediationExitToken::stageis not optional (an unattributed token cannot be constructed), and anAgentExecutor.logsupplied without itsHealthScripts.logorchestrator produces no transactions rather than guessed ones. Theexit_tokens_never_cross_stagestest asserts this across four scenarios.Two smaller distinctions that matter:
Skippedis notNotStarted. It is the stronger claim that detection proved there was nothing to do.Embedded JSON payloads
Preserved losslessly and marked parsed or not. A malformed payload is reported as malformed, never repaired. A payload whose braces do not close inside one record is not captured at all — fragments are never concatenated across records to complete one.
A defect these fixtures found
A Windows path inside a JSON payload arrives escaped as
C:\\Users\\Someone, and the shared profile-segment mask required a single separator — so every such path was exported unmasked. Remediation payloads are the most likely place for that, since detection scripts emit arbitrary data. Fixed in the shared masking with tests.Shared masking (first commit)
The first commit moves the UPN / user-path / command-line masking into
intune::apps::windows::common, so this analyzer and the platform-script analyzer have one implementation rather than two copies that drift.scripts::redact_textstays exported from the same path; no consumer moves. This directly answers the duplication concern raised in review of #386.Fixture matrix
All 17 scenarios required by the issue are under
tests/fixtures/intune/windows/remediations/, each withmanifest.json, its evidence, and a written-outexpected.json.detection-compliant-remediation-skippedadditionally carriesexpected-full.json, a golden of the complete redacted export (regenerate withUPDATE_REMEDIATION_GOLDEN=1).Verification
Run from the repository root:
cargo test --locked -p cmtraceopen-parser— 434 unit (baseline 407), 22 newintune_windows_remediations, 24 scripts, 27 company-portal, 222 esp, 1 doc test. 0 failed.cargo clippy --locked -p cmtraceopen-parser --all-targets -- -D warnings— clean.What is not done
The
event_tracker.rsconsolidation is not in this PR. #360 asks that the relevantHealthScripts/AgentExecutorlogic there be consolidated behind the canonical API. It is not, so two rulesets still exist in the crate. This is the same outstanding gap tracked on #359, and it should be closed once for both analyzers rather than half-done twice. HenceRefs, notCloses.Also out of scope: no native known-source or frontend changes, and no custom-compliance analyzer.
🤖 Generated with Claude Code
Summary by CodeRabbit