feat(intune): model platform-script execution evidence - #386
Merged
Conversation
Before: `ImeLine` exposed line number, timestamp, message, and component. The `thread=` attribute was parsed out of every CCM record but kept private on `ParsedImeRecord`, so it reached `LogEntry` and stopped there. Any consumer of `parse_ime_content` that needed to group a sequence of records belonging to one operation had only adjacency to go on. This moves the field onto `ImeLine` and drops the now-duplicated copy from `ParsedImeRecord`, which reads it back through `entry.line.thread`. This is the right seam because thread is record-level evidence, not a derived display value: it belongs on the record projection alongside component, and keeping one owner avoids the same value living in two structs. The upcoming `intune::apps::windows::scripts` reducer needs it to key interleaved AgentExecutor executions without merging on timestamp proximity, which the issue explicitly forbids. This is an additive public field on a published struct. No in-repo consumer constructs `ImeLine` outside tests; those literals are updated here. Verified with `cargo test --locked -p cmtraceopen-parser`: 355 unit and 222 integration tests pass, identical to the pre-change baseline. Refs #359 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Before: Intune platform-script evidence had no owner. `intune/event_tracker.rs` recognised an `AgentExecutor` source kind and emitted loose events from it, but there was no lifecycle: no way to say whether a policy was received, whether it was launched, whether it exited, or whether the result ever reached the service. A nonzero exit code was just a severity keyword in a line. This adds `cmtraceopen_parser::intune::apps::windows::scripts`, a semantic analyzer over supplied IME evidence: - `sources.rs` classifies each artifact from its name *and* the CCM components actually inside it, so a file called `AgentExecutor.log` full of unrelated records stays `Unknown` and shows up as coverage rather than as evidence; - `rules.rs` classifies one record at a time and decides nothing; - `reducer.rs` groups records into transactions keyed on policy id, run id, and execution context, and reduces each to a state, a last confirmed phase, a typed exit token, an attempt count, a confidence, and the smallest next artifact worth asking for; - `redaction.rs` masks UPNs, user profile segments, and command lines with a stable FNV-1a token, leaving the policy/run GUIDs a reader needs to follow the correlation. This is the right seam because platform scripts, remediations, and Win32 installers reach terminal states for different reasons and must not share one state machine. `HealthScripts` records are recognised as a distinct source and never classified into a platform-script signal; issue #360 owns that lifecycle. Two keying decisions are worth review. Within one AgentExecutor artifact, records inherit the key of their own execution block -- a contiguous run on one CCM thread starting at a launch record -- because that block is written by a single execution. Blocks never span artifacts, so an execution split across a rotation boundary leaves a visible unkeyed tail instead of a guessed join. Separately, a record naming only a policy may adopt a run id when that policy has exactly one run in the supplied evidence; with two or more it keeps its partial key. Neither rule ever merges on timestamp, display name, or a shared `AgentExecutor` component. Verified from the repository root: - `cargo test --locked -p cmtraceopen-parser`: 392 unit (was 355), 222 esp integration, 18 new `intune_windows_scripts` integration, 1 doc test; 0 failed. - `cargo clippy --locked -p cmtraceopen-parser --all-targets -- -D warnings`: clean. The 15 scenarios required by the issue are present under `tests/fixtures/intune/windows/scripts/`, each with a manifest, its evidence, and a written-out `expected.json`. `success-device-context` additionally carries `expected-full.json`, a golden of the complete redacted export that pins the serialized camelCase shape. Closes #359 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 34 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 (7)
📝 WalkthroughWalkthroughAdds a public Windows Intune platform-script analyzer. It classifies supplied artifacts and records, reduces lifecycle evidence into transactions, redacts sensitive data, preserves coverage details, and validates behavior with fixture-driven tests. ChangesWindows Intune platform-script analysis
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a new pure semantic analyzer under cmtraceopen_parser::intune::apps::windows::scripts to model Intune Windows platform-script lifecycle evidence (keying, reduction, coverage, and deterministic redaction), plus a focused fixture-driven integration test matrix. Also exposes CCM thread= on ImeLine to support correlation without timestamp-based joins.
Changes:
- Introduces
intune::apps::windows::scriptsanalyzer (models, source classification, per-record rules, keyed reduction, redaction/export projection). - Adds
intune_windows_scriptsintegration test harness with 15 scenario fixtures + one full redacted golden export. - Makes
ImeLine.thread: Option<u32>public and updates related tests to populate it.
Reviewed changes
Copilot reviewed 46 out of 67 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/cmtraceopen-parser/src/intune/mod.rs | Exposes new intune::apps module. |
| crates/cmtraceopen-parser/src/intune/ime_parser.rs | Adds ImeLine.thread and plumbs parsed thread through. |
| crates/cmtraceopen-parser/src/intune/policy_parser.rs | Updates tests for new ImeLine.thread field. |
| crates/cmtraceopen-parser/src/intune/guid_registry.rs | Updates tests for new ImeLine.thread field. |
| crates/cmtraceopen-parser/src/intune/event_tracker.rs | Updates tests for new ImeLine.thread field. |
| crates/cmtraceopen-parser/src/intune/download_stats.rs | Updates tests for new ImeLine.thread field. |
| crates/cmtraceopen-parser/src/intune/apps/mod.rs | Adds Intune workload analyzer module root. |
| crates/cmtraceopen-parser/src/intune/apps/windows/mod.rs | Adds Windows analyzer namespace. |
| crates/cmtraceopen-parser/src/intune/apps/windows/scripts/mod.rs | Public entrypoint + re-exports for scripts analyzer. |
| crates/cmtraceopen-parser/src/intune/apps/windows/scripts/models.rs | Public serialized contract types for analysis output. |
| crates/cmtraceopen-parser/src/intune/apps/windows/scripts/sources.rs | Artifact source-kind + rotation classification logic. |
| crates/cmtraceopen-parser/src/intune/apps/windows/scripts/rules.rs | Record-level signal/key extraction rules and regexes. |
| crates/cmtraceopen-parser/src/intune/apps/windows/scripts/reducer.rs | Transaction reduction + key reconciliation + coverage. |
| crates/cmtraceopen-parser/src/intune/apps/windows/scripts/redaction.rs | Deterministic/idempotent masking for sensitive strings. |
| crates/cmtraceopen-parser/tests/intune_windows_scripts.rs | Fixture-driven integration tests + golden export check. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/success-device-context/manifest.json | Fixture scenario manifest. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/success-device-context/expected.json | Fixture expectations (contract assertions). |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/success-device-context/expected-full.json | Golden redacted export for shape pinning. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/success-device-context/IntuneManagementExtension.log | Fixture IME evidence. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/success-device-context/AgentExecutor.log | Fixture AgentExecutor evidence. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/success-user-context/manifest.json | Fixture scenario manifest. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/success-user-context/expected.json | Fixture expectations (contract assertions). |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/success-user-context/IntuneManagementExtension.log | Fixture IME evidence. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/success-user-context/AgentExecutor.log | Fixture AgentExecutor evidence. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/nonzero-exit-with-output/manifest.json | Fixture scenario manifest. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/nonzero-exit-with-output/expected.json | Fixture expectations (contract assertions). |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/nonzero-exit-with-output/AgentExecutor.log | Fixture AgentExecutor evidence. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/nonzero-exit-without-output/manifest.json | Fixture scenario manifest. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/nonzero-exit-without-output/expected.json | Fixture expectations (contract assertions). |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/nonzero-exit-without-output/AgentExecutor.log | Fixture AgentExecutor evidence. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/launch-failure/manifest.json | Fixture scenario manifest. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/launch-failure/expected.json | Fixture expectations (contract assertions). |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/launch-failure/AgentExecutor.log | Fixture AgentExecutor evidence. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/timeout/manifest.json | Fixture scenario manifest. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/timeout/expected.json | Fixture expectations (contract assertions). |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/timeout/AgentExecutor.log | Fixture AgentExecutor evidence. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/three-retries-then-success/manifest.json | Fixture scenario manifest. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/three-retries-then-success/expected.json | Fixture expectations (contract assertions). |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/three-retries-then-success/AgentExecutor.log | Fixture AgentExecutor evidence. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/retries-exhausted/manifest.json | Fixture scenario manifest. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/retries-exhausted/expected.json | Fixture expectations (contract assertions). |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/retries-exhausted/IntuneManagementExtension.log | Fixture IME evidence. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/retries-exhausted/AgentExecutor.log | Fixture AgentExecutor evidence. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/execution-success-report-failure/manifest.json | Fixture scenario manifest. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/execution-success-report-failure/expected.json | Fixture expectations (contract assertions). |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/execution-success-report-failure/IntuneManagementExtension.log | Fixture IME evidence. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/execution-success-report-failure/AgentExecutor.log | Fixture AgentExecutor evidence. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/policy-received-no-execution/manifest.json | Fixture scenario manifest. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/policy-received-no-execution/expected.json | Fixture expectations (contract assertions). |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/policy-received-no-execution/IntuneManagementExtension.log | Fixture IME evidence. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/two-scripts-same-minute/manifest.json | Fixture scenario manifest. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/two-scripts-same-minute/expected.json | Fixture expectations (contract assertions). |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/two-scripts-same-minute/AgentExecutor.log | Fixture AgentExecutor evidence. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/multiline-ccm-record/manifest.json | Fixture scenario manifest. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/multiline-ccm-record/expected.json | Fixture expectations (contract assertions). |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/multiline-ccm-record/AgentExecutor.log | Fixture AgentExecutor evidence (multiline record). |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/rotation-boundary/manifest.json | Fixture scenario manifest. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/rotation-boundary/expected.json | Fixture expectations (contract assertions). |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/rotation-boundary/AgentExecutor-1.log | Fixture rotated AgentExecutor evidence. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/rotation-boundary/AgentExecutor.log | Fixture live AgentExecutor evidence (orphan tail). |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/unknown-agent-version/manifest.json | Fixture scenario manifest. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/unknown-agent-version/expected.json | Fixture expectations (contract assertions). |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/unknown-agent-version/AgentExecutor.log | Fixture AgentExecutor evidence (unknown wording). |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/privacy-redaction/manifest.json | Fixture scenario manifest. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/privacy-redaction/expected.json | Fixture expectations + redaction needles. |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/privacy-redaction/IntuneManagementExtension.log | Fixture IME evidence (UPN present). |
| crates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/privacy-redaction/AgentExecutor.log | Fixture AgentExecutor evidence (cmdline/path secrets). |
Suppressed comments (2)
crates/cmtraceopen-parser/src/intune/apps/windows/scripts/sources.rs:217
- If underscore-prefixed archive rotations are represented with
rotation_ordinal = None, the corresponding unit test should assertNonerather thanSome(1).
fn underscore_archive_form_is_recognised_as_a_rotation() {
let (stem, ordinal) = split_rotation("_AgentExecutor.log");
assert_eq!(stem, "AgentExecutor");
assert_eq!(ordinal, Some(1));
}
crates/cmtraceopen-parser/src/intune/apps/windows/scripts/sources.rs:83
- The fallback return path in
split_rotationalso assignsSome(1)for underscore-prefixed archive names. For archives without a trustworthy ordinal, this should beNoneto avoid implying an ordering that isn't present in the filename.
(
without_ext.to_string(),
if underscore_archive { Some(1) } else { Some(0) },
)
Code review of #386 surfaced six real defects. Each is fixed with a regression test that fails without the fix. 1. Unconfirmed artifacts could still mint a transaction. `classify_record` extracted policy and run ids before consulting the source kind, so a file that merely happened to be named `AgentExecutor.log` -- one that `classify_artifact` had correctly refused to confirm -- could create a transaction from any line containing a GUID. This directly contradicted the guarantee in `sources.rs`. Identifiers are now extracted only from a confirmed AgentExecutor or primary IME source. HealthScripts is excluded for the mirror-image reason: it is remediation evidence, and letting it carry a key would attach remediation records to a platform-script transaction before #360 defines the handoff. 2. `normalized_utc` was not what its own doc comment claimed. `ime_parser` fills `timestamp_utc` from the *parsing machine's* local offset when a record carries none, which IME records usually do not. The exported value was therefore UTC-labelled but derived from whoever opened the log. Proof that this was user-visible: the previous golden file had this machine's timezone baked into it and would have failed for any developer elsewhere. `ImeLine` now carries `timezone_offset`, and `normalized_utc` is populated only when the record stated its own offset; `original_offset` reports it. 3. Confidence was demoted bundle-wide. A single unrecognised record anywhere in the supplied evidence dropped *every* transaction to low confidence, including fully evidenced ones for unrelated policies -- contradicting the field's own doc comment. The signal is now scoped to the transaction that contains the record. `unknown_version_observed` remains bundle-level, as coverage. 4. Multi-line records skipped command-line redaction entirely. The regex anchored on `$`, which does not match at a line break, so a `-Command` in a multi-line CCM record matched nothing and exported verbatim. Multi-line records are not hypothetical; a fixture already proves them. 5. Redaction covered only `-Command`/`-EncodedCommand`. `-Password`, `-ApiKey`, `-ClientSecret`, and similar credential switches exported their values in full. The vocabulary now covers them. 6. Transaction ordering ignored time even when time was trustworthy. Ordering decides which attempt a retry sequence reports. When every record in a transaction states its own offset, real time is now used; otherwise source order is kept, and the expectation that callers pass rotations oldest-first is written down rather than assumed. Two fixtures were added: `multi-policy-partial-unknown-version` pins fix 3, and `explicit-timezone-offset` pins fix 2 from the positive direction. Verified from the repository root: - `cargo test --locked -p cmtraceopen-parser`: 397 unit, 222 esp integration, 21 `intune_windows_scripts` integration, 1 doc test; 0 failed. - `cargo clippy --locked -p cmtraceopen-parser --all-targets -- -D warnings`: clean. Refs #359 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three findings from Copilot's review of #386, all valid. 1. `_AgentExecutor.log` reported rotation ordinal `Some(1)`, which is indistinguishable from an explicit `-1` and would mislead ordering. The underscore prefix says a file is an archive, not which archive, so the ordinal is now `None` -- matching what the surrounding comment already claimed. 2. `report_sent_re` ended in the bare fragment `succe`, an accidental truncation that matched any word with that prefix. It now requires a complete success token with a word boundary. 3. `ScriptSourceKind::ScriptOutput` and `PolicyMetadata` were unreachable, and `ScriptTransaction::display_name` was always `None`. Retained output artifacts are named `{policyId}_{runId}.output` / `.error`, which is not a bare word any file could coincidentally carry -- it encodes both halves of a transaction key -- so they are now classified from that identity and satisfy the "retained script output artifact" evidence request for their own transaction. Their *contents* are deliberately never parsed: raw script stdout is unbounded and frequently sensitive, and the file name is the evidence being claimed. `PolicyMetadata` and `display_name` are removed rather than left dead. There is no supplied-metadata input contract in this issue, so neither could ever be populated, and a serialized field that is structurally always null is worse than an absent one. Fixture `nonzero-exit-with-retained-artifact` pins the third fix: the same bundle that asks for an output artifact when it is absent asks for the reporting half instead once it is supplied. Verified from the repository root: - `cargo test --locked -p cmtraceopen-parser`: 399 unit, 222 esp integration, 22 `intune_windows_scripts` integration, 1 doc test; 0 failed. - `cargo clippy --locked -p cmtraceopen-parser --all-targets -- -D warnings`: clean. Refs #359 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 53 out of 77 changed files in this pull request and generated no new comments.
Suppressed comments (1)
crates/cmtraceopen-parser/src/intune/apps/windows/scripts/sources.rs:57
split_rotationreturnsSome(0)("live" ordinal) even when the filename doesn’t end in.log/.LOG(e.g., retained{policyId}_{runId}.error/.outputartifacts). That makes non-log artifacts incorrectly reportrotationOrdinal: 0in coverage, even though the doc comment says the ordinal is only present when the name identifies a rotation/log.
let trimmed = file_name.trim();
let without_ext = trimmed
.strip_suffix(".log")
.or_else(|| trimmed.strip_suffix(".LOG"))
.unwrap_or(trimmed);
![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 (4)
crates/cmtraceopen-parser/src/intune/apps/windows/scripts/sources.rs (1)
89-95: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMatch artifact extensions case-insensitively.
The suffix checks accept only all-lowercase and all-uppercase forms. A file named
{policyId}_{runId}.Outputor.Errorclassifies asScriptSourceKind::Unknown, and the reducer then loses the retained-output evidence for that run. Lines 40-43 have the same gap for.Log.♻️ Proposed case-insensitive extension matching
/// Strip `suffix` from `value`, ignoring ASCII case. fn strip_suffix_ignore_ascii_case<'a>(value: &'a str, suffix: &str) -> Option<&'a str> { let split = value.len().checked_sub(suffix.len())?; let (head, tail) = value.split_at(split); tail.eq_ignore_ascii_case(suffix).then_some(head) }- let stem = trimmed - .strip_suffix(".output") - .or_else(|| trimmed.strip_suffix(".error")) - .or_else(|| trimmed.strip_suffix(".OUTPUT")) - .or_else(|| trimmed.strip_suffix(".ERROR"))?; + let stem = strip_suffix_ignore_ascii_case(trimmed, ".output") + .or_else(|| strip_suffix_ignore_ascii_case(trimmed, ".error"))?;Apply the same helper to the
.logcheck insplit_rotation.🤖 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/scripts/sources.rs` around lines 89 - 95, Make artifact extension matching ASCII-case-insensitive in output_artifact_key and split_rotation. Add and reuse a strip_suffix_ignore_ascii_case helper for .output, .error, and .log checks so mixed-case extensions such as .Output, .Error, and .Log classify and rotate correctly.crates/cmtraceopen-parser/tests/intune_windows_scripts.rs (1)
148-208: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winOptional fixture rules fail open on a key typo.
Each rule runs only when
expected.get(...)returns a value. If a fixture misspellsredactionMustNotContain, the leak assertion never runs and the test still passes. Consider asserting that every key present inexpected.jsonbelongs to a known set, so an unrecognised key fails the scenario.♻️ Proposed guard against unknown expectation keys
+ const KNOWN_KEYS: [&str; 7] = [ + "transactions", + "unkeyedObservationCount", + "coverage", + "messageMustContain", + "timestampMustBe", + "redactionMustNotContain", + "redactionMustContain", + ]; + for key in expected.as_object().expect("expected must be an object").keys() { + assert!( + KNOWN_KEYS.contains(&key.as_str()), + "{scenario}: unknown expectation key {key:?}; a typo would skip its rule" + ); + } + if let Some(rule) = expected.get("messageMustContain") {🤖 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_scripts.rs` around lines 148 - 208, Validate the fixture keys in the scenario expectation handling before applying the optional rules in the test. Define the allowed expectation keys used by the existing checks, including messageMustContain, timestampMustBe, redactionMustNotContain, and redactionMustContain, and fail the scenario when expected contains any unrecognised key.crates/cmtraceopen-parser/src/intune/apps/windows/scripts/redaction.rs (2)
22-30: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winUnsalted FNV-1a tokens are reversible for low-entropy values.
stable_tokenhashes the plaintext directly with a non-keyed 64-bit function. UPNs, profile names, and short inline secrets have low entropy. An attacker who holds an export can recover them with a dictionary attack, because the token depends only on the value.Add a keyed or salted derivation. A build-time or configuration constant preserves cross-run determinism and keeps the
expected-full.jsongolden stable. A per-export random salt gives a stronger guarantee, but it breaks the golden test and cross-export correlation, so choose the tradeoff explicitly and record it in the module docs.♻️ Proposed keyed derivation
-/// FNV-1a. Stable across runs and platforms, which `DefaultHasher` is not. -fn stable_token(kind: &str, value: &str) -> String { - let mut hash: u64 = 0xcbf2_9ce4_8422_2325; - for byte in value.as_bytes() { +/// Keyed FNV-1a. Stable across runs and platforms, which `DefaultHasher` is +/// not, and not invertible by dictionary attack without `REDACTION_KEY`. +const REDACTION_KEY: &[u8] = b"cmtraceopen-script-redaction-v1"; + +fn stable_token(kind: &str, value: &str) -> String { + let mut hash: u64 = 0xcbf2_9ce4_8422_2325; + for byte in REDACTION_KEY.iter().chain(value.as_bytes()) { hash ^= u64::from(*byte); hash = hash.wrapping_mul(0x0000_0100_0000_01b3); } format!("[{kind}:{:016x}]", hash) }🤖 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/scripts/redaction.rs` around lines 22 - 30, Update stable_token to derive its digest with a fixed keyed or salted mechanism instead of hashing value directly with unsalted FNV-1a, preserving deterministic output and the expected-full.json golden. Define the key or salt as a clear module-level configuration constant and document the chosen deterministic tradeoff in the module docs; keep the existing token format and kind handling unchanged.
62-70: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winThe flag rule misses
:and=separators and abbreviated parameters.The pattern requires
\s+after the flag.-Command:Set-Secret hunter2and-Password=hunter2are both valid invocations and neither matches, so the value is exported verbatim. PowerShell also accepts unambiguous prefixes, so-enc <base64>and-Comm <script>bypass the rule.Accept
[:=\s]as the separator and cover the common abbreviations.Note also that
(?P<value>[^\r\n]+)is greedy to the line break. Any diagnostic token that follows the command on the same line is hashed into the token, which conflicts with the module claim that signals and exit codes survive. Confirm that the record grammar never places an exit code after an inline command on one line.♻️ Proposed separator and vocabulary widening
- r"(?i)(?P<flag>-(?:Command|EncodedCommand|Password|Secret|ClientSecret|Token|AccessToken|ApiKey|Api[-_]?Key|Credential|Authorization)\s+)(?P<value>[^\r\n]+)", + r"(?i)(?P<flag>[-/](?:Comm(?:a(?:n(?:d)?)?)?|Enc(?:o(?:d(?:e(?:d(?:C(?:o(?:m(?:m(?:a(?:n(?:d)?)?)?)?)?)?)?)?)?)?)?|Password|Secret|ClientSecret|Token|AccessToken|ApiKey|Api[-_]?Key|Credential|Authorization)[:=\s]\s*)(?P<value>[^\r\n]+)",🤖 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/scripts/redaction.rs` around lines 62 - 70, Update command_line_re to accept colon, equals, or whitespace separators and recognize unambiguous common PowerShell abbreviations such as -enc and -Comm alongside existing parameters. Review the record grammar to determine whether an exit code or diagnostic token can follow an inline command; if so, narrow the value capture so those trailing fields remain available, otherwise preserve the current line-bounded capture only when the grammar guarantees no such suffix.
🤖 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/scripts/redaction.rs`:
- Around line 41-49: Update user_path_re to allow spaces within the captured
user segment, while still excluding path separators, line breaks, quotes, and a
leading “[” so masking remains idempotent. Ensure the replacement re-emits the
matched end boundary as needed, and add a fixture covering a spaced Windows
profile name. Also verify whether down-level names such as CONTOSO\user occur in
classified records and extend upn_re only if that format is in scope.
In `@crates/cmtraceopen-parser/src/intune/apps/windows/scripts/reducer.rs`:
- Around line 369-385: Classify script-output artifacts by name before parsing
their contents. Add the proposed public candidate_source_kind function in
sources.rs, delegating to candidate_from_name, then update the input loop to use
it before parse_ime_content; when it identifies ScriptSourceKind::ScriptOutput,
register output_artifact_identity and continue without parsing, while preserving
the existing content-based classify_artifact path for other inputs.
In `@crates/cmtraceopen-parser/src/intune/apps/windows/scripts/rules.rs`:
- Around line 246-252: Update the hex rendering in the decimal parsing branch
around signed and hex_text so hex_text is produced only when the parsed value
fits the unsigned 32-bit view; avoid casting out-of-range values to u32 and
returning a truncated representation. Preserve the signed decimal value while
leaving hex_text absent for values outside the valid range.
- Around line 417-438: Gate IME classification in the classify_record/reduce
flow on a script-scope marker such as component="PowerShell", so non-script
Win32App records cannot emit ScriptSignal values or policy keys. Preserve
existing classify_ime behavior for records that are in script scope, and add a
fixture covering an equivalent Win32App record without the marker.
In `@crates/cmtraceopen-parser/src/intune/ime_parser.rs`:
- Around line 20-26: Apply the timezone trust gate to every consumer of the
parsed timestamp: update the timestamp selection in download stats and event
tracking so records with timezone_offset == None do not use machine-derived
timestamp_utc for ordering or comparisons. Reuse the source-offset presence
check and preserve timestamp_utc only when its source timezone is trustworthy;
otherwise use each consumer’s existing safe fallback or skip the timestamp-based
operation.
In
`@crates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/launch-failure/expected.json`:
- Around line 7-13: Update the failedToLaunch fixture’s nextEvidenceRequest to
request relevant AgentExecutor.log context or an IntuneManagementExtension.log
record instead of retained script output. Preserve lastConfirmedPhase as
"launched" and leave the other expected fields unchanged.
In
`@crates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/nonzero-exit-with-output/manifest.json`:
- Line 2: Update the manifest description for the nonzero-exit-with-output
fixture to describe evidence from an in-log captured-output record, not a
retained error artifact. Keep the description aligned with the provided
AgentExecutor.log and distinguish it from the
nonzero-exit-with-retained-artifact scenario.
---
Nitpick comments:
In `@crates/cmtraceopen-parser/src/intune/apps/windows/scripts/redaction.rs`:
- Around line 22-30: Update stable_token to derive its digest with a fixed keyed
or salted mechanism instead of hashing value directly with unsalted FNV-1a,
preserving deterministic output and the expected-full.json golden. Define the
key or salt as a clear module-level configuration constant and document the
chosen deterministic tradeoff in the module docs; keep the existing token format
and kind handling unchanged.
- Around line 62-70: Update command_line_re to accept colon, equals, or
whitespace separators and recognize unambiguous common PowerShell abbreviations
such as -enc and -Comm alongside existing parameters. Review the record grammar
to determine whether an exit code or diagnostic token can follow an inline
command; if so, narrow the value capture so those trailing fields remain
available, otherwise preserve the current line-bounded capture only when the
grammar guarantees no such suffix.
In `@crates/cmtraceopen-parser/src/intune/apps/windows/scripts/sources.rs`:
- Around line 89-95: Make artifact extension matching ASCII-case-insensitive in
output_artifact_key and split_rotation. Add and reuse a
strip_suffix_ignore_ascii_case helper for .output, .error, and .log checks so
mixed-case extensions such as .Output, .Error, and .Log classify and rotate
correctly.
In `@crates/cmtraceopen-parser/tests/intune_windows_scripts.rs`:
- Around line 148-208: Validate the fixture keys in the scenario expectation
handling before applying the optional rules in the test. Define the allowed
expectation keys used by the existing checks, including messageMustContain,
timestampMustBe, redactionMustNotContain, and redactionMustContain, and fail the
scenario when expected contains any unrecognised key.
🪄 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: 96a1bba8-9709-42bf-8d0a-fba5a905f183
⛔ Files ignored due to path filters (24)
crates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/execution-success-report-failure/AgentExecutor.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/execution-success-report-failure/IntuneManagementExtension.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/explicit-timezone-offset/AgentExecutor.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/launch-failure/AgentExecutor.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/multi-policy-partial-unknown-version/AgentExecutor.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/multiline-ccm-record/AgentExecutor.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/nonzero-exit-with-output/AgentExecutor.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/nonzero-exit-with-retained-artifact/AgentExecutor.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/nonzero-exit-without-output/AgentExecutor.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/policy-received-no-execution/IntuneManagementExtension.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/privacy-redaction/AgentExecutor.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/privacy-redaction/IntuneManagementExtension.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/retries-exhausted/AgentExecutor.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/retries-exhausted/IntuneManagementExtension.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/rotation-boundary/AgentExecutor-1.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/rotation-boundary/AgentExecutor.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/success-device-context/AgentExecutor.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/success-device-context/IntuneManagementExtension.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/success-user-context/AgentExecutor.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/success-user-context/IntuneManagementExtension.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/three-retries-then-success/AgentExecutor.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/timeout/AgentExecutor.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/two-scripts-same-minute/AgentExecutor.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/unknown-agent-version/AgentExecutor.logis excluded by!**/*.log
📒 Files selected for processing (53)
crates/cmtraceopen-parser/src/intune/apps/mod.rscrates/cmtraceopen-parser/src/intune/apps/windows/mod.rscrates/cmtraceopen-parser/src/intune/apps/windows/scripts/mod.rscrates/cmtraceopen-parser/src/intune/apps/windows/scripts/models.rscrates/cmtraceopen-parser/src/intune/apps/windows/scripts/redaction.rscrates/cmtraceopen-parser/src/intune/apps/windows/scripts/reducer.rscrates/cmtraceopen-parser/src/intune/apps/windows/scripts/rules.rscrates/cmtraceopen-parser/src/intune/apps/windows/scripts/sources.rscrates/cmtraceopen-parser/src/intune/download_stats.rscrates/cmtraceopen-parser/src/intune/event_tracker.rscrates/cmtraceopen-parser/src/intune/guid_registry.rscrates/cmtraceopen-parser/src/intune/ime_parser.rscrates/cmtraceopen-parser/src/intune/mod.rscrates/cmtraceopen-parser/src/intune/policy_parser.rscrates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/execution-success-report-failure/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/execution-success-report-failure/manifest.jsoncrates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/explicit-timezone-offset/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/explicit-timezone-offset/manifest.jsoncrates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/launch-failure/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/launch-failure/manifest.jsoncrates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/multi-policy-partial-unknown-version/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/multi-policy-partial-unknown-version/manifest.jsoncrates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/multiline-ccm-record/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/multiline-ccm-record/manifest.jsoncrates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/nonzero-exit-with-output/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/nonzero-exit-with-output/manifest.jsoncrates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/nonzero-exit-with-retained-artifact/11111111-1111-4111-8111-111111111111_aaaaaaaa-1111-4111-8111-aaaaaaaaaaa1.errorcrates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/nonzero-exit-with-retained-artifact/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/nonzero-exit-with-retained-artifact/manifest.jsoncrates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/nonzero-exit-without-output/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/nonzero-exit-without-output/manifest.jsoncrates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/policy-received-no-execution/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/policy-received-no-execution/manifest.jsoncrates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/privacy-redaction/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/privacy-redaction/manifest.jsoncrates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/retries-exhausted/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/retries-exhausted/manifest.jsoncrates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/rotation-boundary/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/rotation-boundary/manifest.jsoncrates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/success-device-context/expected-full.jsoncrates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/success-device-context/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/success-device-context/manifest.jsoncrates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/success-user-context/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/success-user-context/manifest.jsoncrates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/three-retries-then-success/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/three-retries-then-success/manifest.jsoncrates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/timeout/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/timeout/manifest.jsoncrates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/two-scripts-same-minute/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/two-scripts-same-minute/manifest.jsoncrates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/unknown-agent-version/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/unknown-agent-version/manifest.jsoncrates/cmtraceopen-parser/tests/intune_windows_scripts.rs
Six of seven CodeRabbit findings were valid and are fixed here. The seventh is answered rather than implemented; see the end. 1. A Windows profile name containing a space was only partially masked. Both halves of the `user` capture excluded `\s`, so `C:\Users\John Doe\...` exported as `C:\Users\[user:...] Doe\...` and leaked the surname. The segment is now bounded by the path separator, a quote, or the line break. This is the most serious finding in the batch: it is a live privacy leak on a path Windows permits. 2. Retained output artifacts were parsed before being skipped. The module promises never to read their contents -- they are raw script stdout, which is unbounded and often sensitive -- but the skip ran after `parse_ime_content` had already built an owned line vector for the whole file. Classification is now name-first, so the promise is kept in fact and not only in the comment. 3. An exit code outside the u32 range rendered a truncated hex view: `4294967297` printed as `0x00000001`, a different number than the log recorded. Out-of-range values now carry no hex form at all. 4. The primary IME log is shared by every workload, and record classification did not check the component. A `Win32App` record mentioning "Get policies" and a policy GUID produced a platform-script signal and a policy key -- inventing a script transaction out of app-deployment evidence. Record signals now require a script-scope component. `Win32App` still confirms that the *file* is the IME log; it just cannot speak for this workload. 5. `FailedToLaunch` asked for a "retained script output artifact". A process that never started cannot have written one, so this sent an operator after an artifact that does not exist. It now asks for the surrounding launch context. 6. The `nonzero-exit-with-output` manifest described a retained error artifact it does not contain; that belongs to `nonzero-exit-with-retained-artifact`. Corrected. Not implemented: CodeRabbit also observed that `download_stats` and `event_tracker` still read `timestamp_utc` unconditionally and stay machine-timezone dependent for offset-less records. That is true and pre-existing. `ImeLine::timezone_offset` reports provenance and does not change `timestamp_utc`, so nothing here regresses those paths. Changing how the Intune workspace orders records is a behavioural change that needs its own commit and its own regression coverage, not a field addition. The contract is narrowed in the field's documentation instead, which is the alternative CodeRabbit offered. Verified from the repository root: - `cargo test --locked -p cmtraceopen-parser`: 404 unit, 222 esp integration, 22 `intune_windows_scripts` integration, 1 doc test; 0 failed. - `cargo clippy --locked -p cmtraceopen-parser --all-targets -- -D warnings`: clean. - `git diff --check`: clean. Refs #359 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/scripts/rules.rs`:
- Around line 365-367: Prevent out-of-scope records rejected by
component_is_in_scope in the rules classification flow from being counted as
unclassified_records by reducer.rs, while preserving existing in-scope
classification behavior. Propagate scope status through RecordClassification or
filter these records before the reducer increments the counter. Add an
integration test covering a Win32App IME record and verify it does not increase
script coverage.
🪄 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: 2d23cb3c-c847-4987-9a09-2c5902de8c20
📒 Files selected for processing (8)
crates/cmtraceopen-parser/src/intune/apps/windows/scripts/mod.rscrates/cmtraceopen-parser/src/intune/apps/windows/scripts/redaction.rscrates/cmtraceopen-parser/src/intune/apps/windows/scripts/reducer.rscrates/cmtraceopen-parser/src/intune/apps/windows/scripts/rules.rscrates/cmtraceopen-parser/src/intune/apps/windows/scripts/sources.rscrates/cmtraceopen-parser/src/intune/ime_parser.rscrates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/launch-failure/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/nonzero-exit-with-output/manifest.json
🚧 Files skipped from review as they are similar to previous changes (6)
- crates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/nonzero-exit-with-output/manifest.json
- crates/cmtraceopen-parser/tests/fixtures/intune/windows/scripts/launch-failure/expected.json
- crates/cmtraceopen-parser/src/intune/ime_parser.rs
- crates/cmtraceopen-parser/src/intune/apps/windows/scripts/redaction.rs
- crates/cmtraceopen-parser/src/intune/apps/windows/scripts/reducer.rs
- crates/cmtraceopen-parser/src/intune/apps/windows/scripts/sources.rs
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 53 out of 77 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
crates/cmtraceopen-parser/src/intune/apps/windows/scripts/sources.rs:170
rotation_ordinalis derived viasplit_rotation()for every artifact, includingScriptSourceKind::ScriptOutput. For retained.output/.errorartifacts this will currently reportSome(0)("live file") even though these are not rotated.logfiles, which can mislead consumers interpretingrotation_ordinalas log-rotation metadata.
let candidate = candidate_from_name(&input.file_name);
let (_, rotation_ordinal) = split_rotation(&input.file_name);
Two findings from the second review cycle on #386, both valid. 1. Out-of-scope records inflated platform-script coverage. The previous commit correctly stopped `Win32App` records in the shared IME log from producing a script signal, but the reducer then counted those empty classifications in `unclassified_records`. Because the primary IME log carries every workload, the effect was that a busier device reported worse platform-script coverage than a quiet one, for reasons having nothing to do with scripts. `RecordClassification` now carries `in_scope`, and only in-scope records can be counted as unclassified script records. 2. A transaction could report a stale exit code. `exit_token` was only overwritten when a completion record carried a readable code, so a later completion we could not parse left the earlier attempt's code in place. The state in that situation is `exitedNonZero` -- a completion whose code is unreadable cannot claim success -- so the transaction displayed `exitedNonZero` next to exit code `0`, a contradiction the evidence does not support. The assignment is now unconditional, including `None`. Two fixtures pin the behaviour. `shared-ime-log-other-workload` puts three `Win32App` records in the same IME log as a real script transaction and asserts that script coverage stays at zero. `later-completion-without-readable-code` runs two attempts where the second completion has no readable code and asserts the earlier `0` does not survive. Verified from the repository root: - `cargo test --locked -p cmtraceopen-parser`: 405 unit, 222 esp integration, 24 `intune_windows_scripts` integration, 1 doc test; 0 failed. - `cargo clippy --locked -p cmtraceopen-parser --all-targets -- -D warnings`: clean. Refs #359 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 57 out of 84 changed files in this pull request and generated no new comments.
Suppressed comments (1)
crates/cmtraceopen-parser/src/intune/apps/windows/scripts/sources.rs:170
classify_artifactalways derivesrotation_ordinalviasplit_rotation, even when the candidate isScriptOutput. For retained{policyId}_{runId}.output/.errorartifacts,split_rotationwill default toSome(0)("live"), which is misleading because these artifacts are not rotated logs and their name does not encode an ordinal. This can confuse consumers that display or sort artifacts based onrotation_ordinal.
let candidate = candidate_from_name(&input.file_name);
let (_, rotation_ordinal) = split_rotation(&input.file_name);
Owner
Author
|
Independent verification of the timezone-dependent golden, plus one CI caveat. The TZ bug was real, and Reproduced at
original_offset: line.timezone_offset.map(format_offset),
normalized_utc: line.timezone_offset.and(line.timestamp_utc.clone()),UTC is now only claimed when the source actually carried an offset, and the golden reads Verified at Full crate under Caveat: CI is not actually running these 24 tests. so |
adamgell
added
feature
adamgell
added a commit
that referenced
this pull request
Aug 1, 2026
) * refactor(intune): give the Windows analyzers one masking implementation 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> * feat(intune): add the remediation detection/remediation analyzer 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> * fix(intune): close four defects found by review of the remediation analyzer 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> * fix(intune): close nine review findings on the remediation analyzer 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> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This was referenced Aug 2, 2026
3 tasks
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 #359. Deliberately not
Closes— see "What is not done" at the bottom.What this adds
cmtraceopen_parser::intune::apps::windows::scripts— a semantic analyzer over supplied IME evidence for the Intune platform-script lifecycle. It separates policy receipt, scheduling, process launch, output, exit status, retry behaviour, and service reporting, rather than treating a severity keyword or an exit code as the diagnosis.sources.rsrules.rsreducer.rsredaction.rsmodels.rsTwo keying decisions worth reviewing
Execution blocks. Within one AgentExecutor artifact, records inherit the key of their own execution block — a contiguous run on one CCM thread beginning at a launch record — because a block is written by a single execution. Blocks never span artifacts, so an execution split across a rotation boundary leaves a visible unkeyed tail rather than a guessed join (fixture
rotation-boundary).Partial-key reconciliation. The primary IME log names a policy but almost never names the run; AgentExecutor names both. A policy-only record may adopt a run id when that policy has exactly one run in the supplied evidence. With two or more runs it keeps its partial key and stays ambiguous. Without this the reporting half of the lifecycle could never join its own execution.
Neither rule merges on timestamp, display name, or a shared
AgentExecutorcomponent.Public API change
ImeLinegains two fields, both already parsed on every CCM record but previously kept private onParsedImeRecord(the duplicate copies are removed):thread: Option<u32>— lets the reducer key interleaved executions instead of guessing from adjacency.timezone_offset: Option<i32>— lets a consumer tell a source-stated offset from the parser's local-timezone fallback.Additive on a published struct; no in-repo consumer constructs
ImeLineoutside tests.Defects found and fixed by review
The second commit fixes six defects that review surfaced, each with a regression test:
AgentExecutor.logcould create a transaction from any line containing a GUID — contradictingsources.rs's own stated guarantee.normalized_utcwas not what its doc comment claimed.ime_parserfillstimestamp_utcfrom the parsing machine's local offset when a record carries none, which IME records usually do not. The old golden file had this machine's timezone baked into it and would have failed for any developer in another timezone.$, which does not match at a line break.-Command/-EncodedCommand—-Password,-ApiKey,-ClientSecretexported verbatim.Fixture matrix
The 15 scenarios required by the issue are under
tests/fixtures/intune/windows/scripts/, each withmanifest.json, its evidence files, and a written-outexpected.jsonso a reviewer can see what is being asserted. Two more were added for the review fixes (multi-policy-partial-unknown-version,explicit-timezone-offset).success-device-contextadditionally carriesexpected-full.json, a golden of the complete redacted export (regenerate withUPDATE_SCRIPT_GOLDEN=1).Verification
Run from the repository root:
cargo test --locked -p cmtraceopen-parser— 397 unit (baseline 355), 222 esp integration, 21intune_windows_scriptsintegration, 1 doc test. 0 failed.cargo clippy --locked -p cmtraceopen-parser --all-targets -- -D warnings— clean.cargo test --locked --manifest-path src-tauri/Cargo.toml --all-features— 427 + 180 + 82 + others, 0 failed.cargo clippy --locked -p cmtrace-open --all-targets --all-features -- -D warnings— clean.npx tsc --noEmit— exit 0 (unchanged; no frontend surface in this PR).What is not done
The
event_tracker.rsconsolidation the issue asks for is not in this PR. #359 says to "migrate or wrap the relevantImeSourceKind::AgentExecutorbehavior fromintune/event_tracker.rsso there is one ruleset". That has not happened:event_tracker.rsstill has its own source classification and its own policy-id, exit-code, and timeout regexes, and those regexes are not equivalent to the new ones (its exit-code pattern is decimal-only; the new one also reads hex). Two rulesets exist in the crate today. This PR usesRefsrather thanClosesso #359 stays open for that work.Also out of scope here: no native known-source or frontend changes (the analyzer is pure and consumes artifacts the caller already read and decoded), and no remediation pairing —
HealthScriptsis recognised as a distinct source and deliberately carries no key, pending #360.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests