feat(intune): land the parser-family skeleton for epic #356 - #388
Merged
Conversation
📝 WalkthroughWalkthroughThis change adds shared Intune evidence and normalized-data contracts, exposes planned parser namespaces, adds fixture contract validation, and runs parser tests and Clippy in CI. ChangesIntune parser foundation
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
adamgell
force-pushed
the
claude/356-intune-parser-family-skeleton
branch
from
July 31, 2026 05:29
a266023 to
99f2395
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR lands the initial “parser-family skeleton” for Intune epic #356 by introducing the new workload-first module namespace tree (apps/, enrollment/, device/, portal/) along with shared contracts (intune::evidence, intune::normalized) and a shared fixture validation harness intended to keep future leaf PRs isolated. It also updates CI so the cmtraceopen-parser crate’s tests and clippy are actually gated in the main check job.
Changes:
- Add the Intune workload namespace skeleton (reserved leaf modules per child issue) plus shared
evidence+normalizedcontracts incmtraceopen-parser. - Add a shared Intune fixture contract harness and a pinned reference
_skeletoncorpus + contract tests. - Update CI to run
cargo test -p cmtraceopen-parserandcargo clippy -p cmtraceopen-parserin the main workflow.
Reviewed changes
Copilot reviewed 43 out of 45 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src-tauri/src/intune/mod.rs | Re-export workload-root namespaces from the parser crate to avoid repeated edits as leaves land. |
| crates/cmtraceopen-parser/tests/support/mod.rs | Adds shared fixture-contract validation harness (manifest/evidence/privacy scanning). |
| crates/cmtraceopen-parser/tests/intune_skeleton_contract.rs | Adds contract tests that pin the reference corpus layout and validate harness rejection behavior. |
| crates/cmtraceopen-parser/tests/fixtures/intune/_skeleton/reference-scenario/manifest.json | Adds reference scenario manifest for the skeleton corpus. |
| crates/cmtraceopen-parser/tests/fixtures/intune/_skeleton/reference-scenario/expected.json | Adds reference scenario expected outputs/coverage/findings. |
| crates/cmtraceopen-parser/tests/fixtures/intune/_skeleton/reference-scenario/evidence/skeleton-ime/current/IntuneManagementExtension.log | Adds synthetic marker-bearing reference evidence file. |
| crates/cmtraceopen-parser/src/intune/mod.rs | Introduces workload namespaces and documents coexistence with legacy flat IME modules. |
| crates/cmtraceopen-parser/src/intune/evidence.rs | Adds shared evidence/provenance/coverage/finding contracts for the new Intune family. |
| crates/cmtraceopen-parser/src/intune/normalized.rs | Adds platform-neutral normalized Windows event/report input contracts (shared across multiple issues). |
| crates/cmtraceopen-parser/src/intune/apps/mod.rs | Adds top-level Intune “apps” workload namespace. |
| crates/cmtraceopen-parser/src/intune/apps/windows/mod.rs | Adds Windows apps subtree and reserves leaf modules. |
| crates/cmtraceopen-parser/src/intune/apps/windows/microsoft_store/mod.rs | Reserves Microsoft Store evidence leaf (issue #358). |
| crates/cmtraceopen-parser/src/intune/apps/windows/remediations/mod.rs | Reserves remediations evidence leaf (issue #360). |
| crates/cmtraceopen-parser/src/intune/apps/windows/scripts/mod.rs | Reserves scripts evidence leaf (issue #359). |
| crates/cmtraceopen-parser/src/intune/apps/windows/win32/mod.rs | Reserves Win32 deployment evidence leaf (issue #357). |
| crates/cmtraceopen-parser/src/intune/apps/macos/mod.rs | Adds macOS apps subtree and reserves leaf modules. |
| crates/cmtraceopen-parser/src/intune/apps/macos/pkg/mod.rs | Reserves macOS package deployment leaf (issue #361). |
| crates/cmtraceopen-parser/src/intune/apps/macos/shell_scripts/mod.rs | Reserves macOS shell script execution leaf (issue #361). |
| crates/cmtraceopen-parser/src/intune/enrollment/mod.rs | Adds top-level Intune “enrollment” workload namespace. |
| crates/cmtraceopen-parser/src/intune/enrollment/windows/mod.rs | Adds Windows enrollment subtree. |
| crates/cmtraceopen-parser/src/intune/enrollment/windows/autopilot/mod.rs | Reserves Autopilot-outside-ESP leaf (issue #362). |
| crates/cmtraceopen-parser/src/intune/device/mod.rs | Adds top-level Intune “device” workload namespace. |
| crates/cmtraceopen-parser/src/intune/device/windows/mod.rs | Adds Windows device subtree and reserves leaf modules. |
| crates/cmtraceopen-parser/src/intune/device/windows/compliance/mod.rs | Reserves compliance evidence leaf (issue #364). |
| crates/cmtraceopen-parser/src/intune/device/windows/configuration/mod.rs | Reserves configuration evidence leaf (issue #363). |
| crates/cmtraceopen-parser/src/intune/device/windows/inventory/mod.rs | Reserves device inventory agent leaf (issue #354). |
| crates/cmtraceopen-parser/src/intune/device/windows/updates/mod.rs | Reserves WUfB evidence leaf (issue #365). |
| crates/cmtraceopen-parser/src/intune/portal/mod.rs | Adds top-level Intune “portal” workload namespace. |
| crates/cmtraceopen-parser/src/intune/portal/android/mod.rs | Adds Android portal subtree. |
| crates/cmtraceopen-parser/src/intune/portal/android/company_portal/mod.rs | Adds Android Company Portal subtree. |
| crates/cmtraceopen-parser/src/intune/portal/android/company_portal/diagnostics/mod.rs | Reserves Android imported diagnostics leaf (issue #371). |
| crates/cmtraceopen-parser/src/intune/portal/ios_ipados/mod.rs | Adds iOS/iPadOS portal subtree. |
| crates/cmtraceopen-parser/src/intune/portal/ios_ipados/company_portal/mod.rs | Adds iOS/iPadOS Company Portal subtree. |
| crates/cmtraceopen-parser/src/intune/portal/ios_ipados/company_portal/diagnostics/mod.rs | Reserves iOS/iPadOS console diagnostics leaf (issue #372). |
| crates/cmtraceopen-parser/src/intune/portal/macos/mod.rs | Adds macOS portal subtree. |
| crates/cmtraceopen-parser/src/intune/portal/macos/company_portal/mod.rs | Adds macOS Company Portal subtree and reserves leaves. |
| crates/cmtraceopen-parser/src/intune/portal/macos/company_portal/diagnostics/mod.rs | Reserves macOS saved diagnostics leaf (issue #369). |
| crates/cmtraceopen-parser/src/intune/portal/macos/company_portal/unified_log/mod.rs | Reserves macOS unified-log leaf (issue #370). |
| crates/cmtraceopen-parser/src/intune/portal/windows/mod.rs | Adds Windows portal subtree. |
| crates/cmtraceopen-parser/src/intune/portal/windows/company_portal/mod.rs | Adds Windows Company Portal subtree and reserves leaves. |
| crates/cmtraceopen-parser/src/intune/portal/windows/company_portal/package_state/mod.rs | Reserves deterministic package-state facts leaf (issue #367). |
| .github/workflows/cmtrace-ci.yml | Ensures parser-crate tests and clippy run in CI (not just src-tauri package). |
| .claude/skills/gh-copilot-review-loop/SKILL.md | Adds a Copilot review-loop skill description. |
| .claude/skills/gh-copilot-review-loop/scripts/review_state.py | Adds a helper script to fetch PR review/thread state via GraphQL. |
| .claude/skills/gh-copilot-review-loop/.gitignore | Ignores Python cache artifacts for the skill. |
| .claude/skills/batch-issue-prs/SKILL.md | Adds batch issue → PR workflow guidance for agent usage. |
| @@ -0,0 +1,4 @@ | |||
| //! Windows Company Portal application evidence. | |||
|
|
|||
| pub mod logs; | |||
| //! macOS Company Portal application evidence. | ||
|
|
||
| pub mod diagnostics; | ||
| pub mod logs; |
Comment on lines
+38
to
+41
| /// Capture states a manifest artifact may declare. | ||
| /// | ||
| /// These mirror `IntuneAccessState` so a fixture can express "this source was | ||
| /// capped" or "this source was skipped" rather than collapsing both to absent. |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 43 out of 45 changed files in this pull request and generated no new comments.
Suppressed comments (4)
crates/cmtraceopen-parser/tests/support/mod.rs:46
- The manifest captureState token uses "accessDenied", but the rest of the Intune evidence/coverage vocabulary (and the coverage mapping right below) uses "permissionDenied". Keeping two different strings for the same concept will make fixtures and leaf reducers harder to keep consistent.
crates/cmtraceopen-parser/tests/support/mod.rs:155 - This error branch panics because the file could not be read, but the message currently says it "is readable", which is easy to misread when debugging fixture failures.
crates/cmtraceopen-parser/tests/support/mod.rs:76 - After renaming the captureState token to "permissionDenied", the coverage binding table should use the same key; otherwise permission-denied artifacts won't have their required coverage status enforced.
crates/cmtraceopen-parser/tests/support/mod.rs:111 - The panic message fires when read_dir fails, but the string says the directory "is readable", which is confusing when diagnosing fixture path problems.
This issue also appears on line 155 of the same file.
adamgell
added
feature
Before this change `cmtraceopen_parser::intune` was a flat list of seven IME-centered modules. Epic #356 reorganizes it into workload namespaces (apps, enrollment, device, portal) across seventeen child issues. Implemented naively, every one of those issues would edit the same handful of mod.rs files, so no two could merge independently. This commit creates the whole tree up front, with every leaf reserved as a documented placeholder owned by exactly one issue. A child PR then touches only its own leaf directory, its own tests/*.rs file, and its own fixture subtree. Two contracts are shared and so live here rather than in any one leaf: - intune::evidence carries the provenance envelope every observation rides on. It distinguishes absent, inaccessible, capped, skipped, unsupported, and malformed evidence instead of collapsing all six into "no data", and its finding type records the invariant that a conclusion must cite either evidence or a coverage gap. - intune::normalized holds the platform-neutral Windows event and setting report inputs. Issues #358, #363, #364, and #365 all need these, and #364 and #365 are explicitly told to reuse #363's; leaving them in #363 would block the other three behind it. tests/support/ adds the shared fixture harness so fifteen leaves do not each copy ~300 lines of manifest/expected/evidence validation, and the reference corpus under tests/fixtures/intune/_skeleton/ pins the layout they follow. The CI change is the one that matters most. Every Rust step in the check job runs with working-directory: src-tauri, which selects only the cmtrace-open package, and the sole parser test in CI was the Windows-only `--test esp_diagnostics`. New parser test targets would not have run at all, and the parser crate had no clippy gate. Both are now wired into the check job once, so each leaf lands gated without editing a workflow. Behavior is unchanged: no existing module, type, or public path is modified, and intune::models keeps its hand-written 13-field Serialize impl untouched. Verified: cargo test --locked -p cmtraceopen-parser -> 362 + 222 + 18 passed, 0 failed cargo clippy --locked -p cmtraceopen-parser --all-targets -- -D warnings -> clean cargo check --locked --manifest-path src-tauri/Cargo.toml --all-targets -> Finished npx tsc --noEmit -> exit 0 The 18 new tests include 13 adversarial mutations proving the fixture validator rejects version drift, byte-count lies, path escapes, duplicate artifact ids, coverage omissions, and uncited findings. Refs #356 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…nner
The scanner tokenized only on whitespace, quotes, and commas. A semicolon-
joined recipient list therefore arrived as a single token, and after
split_once('@') its domain still contained ';' and a second '@'. That failed
the final character check, so the token was skipped and BOTH real addresses
passed the tripwire undetected.
A pasted mail header is exactly the shape most likely to carry real customer
addresses into a fixture, so this was the case the scanner most needed to
catch and the one it missed.
Widened the delimiter set to the punctuation that actually surrounds an
address in log, JSON, and header text, and added a regression test covering
semicolon, colon, angle-bracket, bracket, equals, and pipe forms.
Verified: cargo test --locked -p cmtraceopen-parser --test intune_skeleton_contract
-> 19 passed, 0 failed
cargo clippy --locked -p cmtraceopen-parser --all-targets -- -D warnings -> clean
Refs #356
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`.gitignore` carries `Logs/` for test output. macOS and Windows match that case-insensitively, so it also matched the `logs` leaf modules of the Intune Company Portal tree and left both untracked. The build passed locally, where the untracked files still exist on disk, and the pushed branch was already broken: cloning it produces a portal/macos/company_portal/ containing only diagnostics, mod.rs, and unified_log, while that mod.rs declares `pub mod logs;`. A fresh checkout cannot compile. Verified by cloning the pushed branch into a scratch directory and listing the tree. This would also have blocked issues #366 and #368, whose entire implementations live under those two directories, and the failure would have appeared as an inexplicable CI-only break. Re-include the directories before their contents; git cannot re-include a file whose parent directory is still excluded. Refs #356 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Every one of these is a silent pass rather than a crash, which is the dangerous direction: fifteen leaf corpora would have validated clean while asserting things that were not true. 1. .gitignore only re-included the src/ side of the `logs` problem, so the fixture corpora for #366 and #368 were still swallowed. Replaced the per-path negations with a root anchor (/Logs/). Anchoring fixes the class; negations only ever cover the paths someone remembered. Verified all three cases: fixture logs tracked, src logs tracked, root Logs/ still ignored. 2. Coverage status was never read. A manifest could declare an artifact absent while expected.json declared it available, and any finding citing that entry inherited the lie. Coverage status is now bound to capture state through an explicit table. 3. parseFailed and unsupported artifacts were forced to have no file, but parseFailed means the artifact was collected and could not be interpreted -- the malformed bytes are the point. Epic #356 requires a malformed scenario from every child issue, so the mandated shape was inexpressible. 4. The privacy scan never ran on manifest.json or expected.json, which are hand written and hold the free-text fields where a real path or identity gets typed by mistake. 5. Writing the test for (4) exposed a further bug: inside JSON a Windows path is necessarily escaped as `C:\\Users\\`, so searching for the literal `C:\Users\` never matched. The descriptors leaked exactly the paths the forbidden list exists to block. Both forms are now checked. 6. scenario_names returned an empty vec for a missing corpus directory, so a misspelled path produced a loop that ran zero times and a test that reported success having validated nothing. It now panics with the path. 7. path_safety_problem was lexical only, so a symlink under evidence/ escaped the scenario root: metadata followed it, read_to_string read the host file, and the privacy scan ran against content outside the corpus. Targets are now resolved and confined to the scenario directory. Verified: cargo test --locked -p cmtraceopen-parser -> 362 + 222 + 26 passed, 0 failed cargo clippy --locked -p cmtraceopen-parser --all-targets -- -D warnings -> clean cargo check --locked --manifest-path src-tauri/Cargo.toml --all-targets -> Finished npx tsc --noEmit -> exit 0 Seven new tests, each asserting the validator rejects the specific corpus it previously accepted. Refs #356 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
adamgell
force-pushed
the
claude/356-intune-parser-family-skeleton
branch
from
July 31, 2026 18:00
29ddc36 to
d852236
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
crates/cmtraceopen-parser/tests/intune_skeleton_contract.rs (1)
49-62: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a rejection test for evidence closure.
The mutation set covers envelope, path, capture-state, coverage, and finding rules. It does not cover
validate_evidence_closure. That rule is one the fifteen workload leaves will rely on, so an untested branch there can regress silently. Add a scratch scenario that writes an unreferenced file underevidence/and assert the failure mentions "is not referenced by any manifest artifact".🤖 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_skeleton_contract.rs` around lines 49 - 62, Add an adversarial scratch scenario alongside the existing mutation tests that creates an unreferenced file beneath evidence/. Validate it through the same helper used by the other mutations and assert rejection via assert_rejected, checking for the message “is not referenced by any manifest artifact” to cover validate_evidence_closure.crates/cmtraceopen-parser/tests/fixtures/intune/_skeleton/reference-scenario/manifest.json (1)
16-17: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winRetain the manifest entry and protect the fixture’s line endings. The file is tracked, contains the synthetic marker, and is exactly 220 bytes. Git currently records and checks out LF endings, but no attribute protects this path from future conversion. Add a
-textrule if byte-stable checkouts are required.🤖 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/fixtures/intune/_skeleton/reference-scenario/manifest.json` around lines 16 - 17, Add a Git attributes rule for the fixture path represented by the manifest entry so the tracked IntuneManagementExtension.log remains byte-stable with LF line endings and is not subject to future text conversion. Retain the existing manifest entry and ensure the rule uses the repository’s established attributes location and syntax.
🤖 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/normalized.rs`:
- Around line 48-62: Clarify the event-identity contract for
NormalizedWindowsEvent by adding a doc comment that explicitly identifies
whether its top-level channel, provider, event_id, and record_id fields or
context.provenance.event is authoritative. Ensure the comment addresses the
relationship to provenance event data and record_number so child implementations
use one consistent source of truth.
In `@crates/cmtraceopen-parser/tests/support/mod.rs`:
- Around line 386-426: The evidence validation at
crates/cmtraceopen-parser/tests/support/mod.rs:386-426 must canonicalize each
file and verify containment within scenario_root before any metadata or
read_to_string calls; skip reading and report the containment failure when it
resolves outside the scenario. At
crates/cmtraceopen-parser/tests/support/mod.rs:131-150, replace the
path.is_dir() traversal check with symlink_metadata so directory symlinks are
not descended into.
---
Nitpick comments:
In
`@crates/cmtraceopen-parser/tests/fixtures/intune/_skeleton/reference-scenario/manifest.json`:
- Around line 16-17: Add a Git attributes rule for the fixture path represented
by the manifest entry so the tracked IntuneManagementExtension.log remains
byte-stable with LF line endings and is not subject to future text conversion.
Retain the existing manifest entry and ensure the rule uses the repository’s
established attributes location and syntax.
In `@crates/cmtraceopen-parser/tests/intune_skeleton_contract.rs`:
- Around line 49-62: Add an adversarial scratch scenario alongside the existing
mutation tests that creates an unreferenced file beneath evidence/. Validate it
through the same helper used by the other mutations and assert rejection via
assert_rejected, checking for the message “is not referenced by any manifest
artifact” to cover validate_evidence_closure.
🪄 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: d2d40d29-117f-4ccf-8abc-f03fab65a506
⛔ Files ignored due to path filters (1)
crates/cmtraceopen-parser/tests/fixtures/intune/_skeleton/reference-scenario/evidence/skeleton-ime/current/IntuneManagementExtension.logis excluded by!**/*.log
📒 Files selected for processing (41)
.github/workflows/cmtrace-ci.yml.gitignorecrates/cmtraceopen-parser/src/intune/apps/macos/mod.rscrates/cmtraceopen-parser/src/intune/apps/macos/pkg/mod.rscrates/cmtraceopen-parser/src/intune/apps/macos/shell_scripts/mod.rscrates/cmtraceopen-parser/src/intune/apps/mod.rscrates/cmtraceopen-parser/src/intune/apps/windows/microsoft_store/mod.rscrates/cmtraceopen-parser/src/intune/apps/windows/mod.rscrates/cmtraceopen-parser/src/intune/apps/windows/remediations/mod.rscrates/cmtraceopen-parser/src/intune/apps/windows/win32/mod.rscrates/cmtraceopen-parser/src/intune/device/mod.rscrates/cmtraceopen-parser/src/intune/device/windows/compliance/mod.rscrates/cmtraceopen-parser/src/intune/device/windows/configuration/mod.rscrates/cmtraceopen-parser/src/intune/device/windows/inventory/mod.rscrates/cmtraceopen-parser/src/intune/device/windows/mod.rscrates/cmtraceopen-parser/src/intune/device/windows/updates/mod.rscrates/cmtraceopen-parser/src/intune/enrollment/mod.rscrates/cmtraceopen-parser/src/intune/enrollment/windows/autopilot/mod.rscrates/cmtraceopen-parser/src/intune/enrollment/windows/mod.rscrates/cmtraceopen-parser/src/intune/evidence.rscrates/cmtraceopen-parser/src/intune/mod.rscrates/cmtraceopen-parser/src/intune/normalized.rscrates/cmtraceopen-parser/src/intune/portal/android/company_portal/diagnostics/mod.rscrates/cmtraceopen-parser/src/intune/portal/android/company_portal/mod.rscrates/cmtraceopen-parser/src/intune/portal/android/mod.rscrates/cmtraceopen-parser/src/intune/portal/ios_ipados/company_portal/diagnostics/mod.rscrates/cmtraceopen-parser/src/intune/portal/ios_ipados/company_portal/mod.rscrates/cmtraceopen-parser/src/intune/portal/ios_ipados/mod.rscrates/cmtraceopen-parser/src/intune/portal/macos/company_portal/diagnostics/mod.rscrates/cmtraceopen-parser/src/intune/portal/macos/company_portal/logs/mod.rscrates/cmtraceopen-parser/src/intune/portal/macos/company_portal/mod.rscrates/cmtraceopen-parser/src/intune/portal/macos/company_portal/unified_log/mod.rscrates/cmtraceopen-parser/src/intune/portal/macos/mod.rscrates/cmtraceopen-parser/src/intune/portal/mod.rscrates/cmtraceopen-parser/src/intune/portal/windows/company_portal/logs/mod.rscrates/cmtraceopen-parser/src/intune/portal/windows/company_portal/mod.rscrates/cmtraceopen-parser/tests/fixtures/intune/_skeleton/reference-scenario/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/intune/_skeleton/reference-scenario/manifest.jsoncrates/cmtraceopen-parser/tests/intune_skeleton_contract.rscrates/cmtraceopen-parser/tests/support/mod.rssrc-tauri/src/intune/mod.rs
Comment on lines
+48
to
+62
| #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] | ||
| #[serde(rename_all = "camelCase")] | ||
| pub struct NormalizedWindowsEvent { | ||
| pub context: IntuneObservationContext, | ||
| pub channel: String, | ||
| pub provider: String, | ||
| pub event_id: u32, | ||
| pub level: NormalizedEventLevel, | ||
| pub task: Option<String>, | ||
| pub keywords: Option<String>, | ||
| pub record_id: Option<u64>, | ||
| pub activity_id: Option<String>, | ||
| pub named_data: Vec<IntuneNamedValue>, | ||
| pub message: Option<String>, | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Clarify or remove the duplicate event-identity fields between NormalizedWindowsEvent and IntuneEventProvenance.
NormalizedWindowsEvent carries its own channel, provider, event_id, and record_id fields. The evidence contracts already define a IntuneProvenance struct with an event: Option<IntuneEventProvenance> field, where IntuneEventProvenance carries channel, provider, event_id, and record_id, described as the coordinates an observation came from "when the source is an event log." Every NormalizedWindowsEvent is by definition sourced from an event log, so these two representations of the same identity data can diverge if a leaf populates one but not the other.
The round-trip test compounds this: it sets context.provenance.event to None while independently populating record_id: Some(7) on NormalizedWindowsEvent, and separately sets context.provenance.record_number: Some(7) in the same context object. Nothing enforces that these three values stay consistent.
State explicitly, in a doc comment, which field is authoritative (top-level fields on NormalizedWindowsEvent, or context.provenance.event), or drop the redundant one. Do this now, before the 17 planned child-issue implementations each populate this contract independently.
🤖 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/normalized.rs` around lines 48 - 62,
Clarify the event-identity contract for NormalizedWindowsEvent by adding a doc
comment that explicitly identifies whether its top-level channel, provider,
event_id, and record_id fields or context.provenance.event is authoritative.
Ensure the comment addresses the relationship to provenance event data and
record_number so child implementations use one consistent source of truth.
Comment on lines
+386
to
+426
| match std::fs::read_to_string(&file_path) { | ||
| Ok(contents) => { | ||
| let first_line = contents.lines().next().unwrap_or_default(); | ||
| failures.require(first_line.contains(SYNTHETIC_MARKER), || { | ||
| format!( | ||
| "{scenario}/{id}: first line of {relative_path} must contain {SYNTHETIC_MARKER:?}" | ||
| ) | ||
| }); | ||
| failures.absorb(privacy_problems( | ||
| &format!("{scenario}/{id}:{relative_path}"), | ||
| &contents, | ||
| )); | ||
| } | ||
| Err(error) => failures.push(format!( | ||
| "{scenario}/{id}: {relative_path} is not readable as UTF-8: {error}" | ||
| )), | ||
| } | ||
|
|
||
| // Resolve symlinks and confirm the target is still inside the scenario. | ||
| // The component check above is lexical only, so without this a symlink | ||
| // under evidence/ reads host files and the privacy scan runs against | ||
| // content that is not in the corpus at all. | ||
| match (file_path.canonicalize(), scenario_root.canonicalize()) { | ||
| (Ok(canonical), Ok(root)) => { | ||
| if canonical.starts_with(&root) { | ||
| failures.require(referenced.insert(canonical), || { | ||
| format!( | ||
| "{scenario}/{id}: {relative_path} is referenced by more than one artifact" | ||
| ) | ||
| }); | ||
| } else { | ||
| failures.push(format!( | ||
| "{scenario}/{id}: {relative_path} resolves to {}, outside the scenario directory", | ||
| canonical.display() | ||
| )); | ||
| } | ||
| } | ||
| _ => failures.push(format!( | ||
| "{scenario}/{id}: {relative_path} could not be resolved to a real path" | ||
| )), | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
The harness follows symlinks before it verifies containment. Both sites trust the lexical path check and then use symlink-following filesystem calls, so a committed symlink inside evidence/ reaches host content before any rejection.
crates/cmtraceopen-parser/tests/support/mod.rs#L386-L426: move thecanonicalizecontainment check above themetadataandread_to_stringcalls, and skip reading when the canonical path is outsidescenario_root. This keeps host SIDs and email addresses out of the failure messages that CI prints.crates/cmtraceopen-parser/tests/support/mod.rs#L131-L150: replacepath.is_dir()with asymlink_metadatacheck so the walk does not descend through a directory symlink. A link such asevidence/self -> ..currently makes the loop run forever.
📍 Affects 1 file
crates/cmtraceopen-parser/tests/support/mod.rs#L386-L426(this comment)crates/cmtraceopen-parser/tests/support/mod.rs#L131-L150
🤖 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/support/mod.rs` around lines 386 - 426, The
evidence validation at crates/cmtraceopen-parser/tests/support/mod.rs:386-426
must canonicalize each file and verify containment within scenario_root before
any metadata or read_to_string calls; skip reading and report the containment
failure when it resolves outside the scenario. At
crates/cmtraceopen-parser/tests/support/mod.rs:131-150, replace the
path.is_dir() traversal check with symlink_metadata so directory symlinks are
not descended into.
adamgell
added a commit
that referenced
this pull request
Jul 31, 2026
`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>
adamgell
added a commit
that referenced
this pull request
Aug 1, 2026
Resolves the overlap between this branch and the parser-family skeleton landed by #388. Conflicts, all resolved as the union of both sides: - crates/cmtraceopen-parser/src/intune/mod.rs: took main's module list, which already contains the `device` module this branch added plus the apps/enrollment/portal/evidence/normalized modules from the skeleton. - crates/cmtraceopen-parser/src/intune/device/mod.rs: took main's, a strict superset (same `pub mod windows;` plus the module doc). - crates/cmtraceopen-parser/src/intune/device/windows/mod.rs: took main's, which declares `inventory` alongside the sibling compliance, configuration, and updates slots. Git could not see a fourth overlap because the two sides spelled the same module path differently: this branch added `inventory.rs` while the skeleton reserved `inventory/mod.rs`. Keeping both would be ambiguous module resolution. The implementation now lives in `inventory/mod.rs`, matching the directory layout of every sibling leaf, and the reserved slot's doc header is folded into it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This was referenced Aug 1, 2026
18 tasks
This was referenced Aug 3, 2026
This was referenced Aug 10, 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.
Why
Epic #356 reorganizes
cmtraceopen_parser::intunefrom a flat, IME-centered list of seven modules into workload namespaces (apps,enrollment,device,portal) across seventeen child issues.Implemented naively, every one of those issues would edit the same handful of
mod.rsfiles, so no two child PRs could merge independently. Epic #356's own dependency order names this as step 1: "land the parser-family skeleton and canonical compatibility façades."This PR is that step. It creates the entire tree up front, with every leaf reserved as a documented placeholder owned by exactly one issue. A child PR then touches only its own leaf directory, its own
tests/*.rs, and its own fixture subtree.What is shared, and why it lives here
intune::evidencecarries the provenance envelope every observation rides on. It distinguishes absent, inaccessible, capped, skipped, unsupported, and malformed evidence instead of collapsing all six into "no data", andIntuneFindingrecords the invariant that a conclusion must cite either evidence or a coverage gap.intune::normalizedholds the platform-neutral Windows event and setting-report inputs. Issues Intune Windows: model Microsoft Store app evidence #358, Intune Windows device: model configuration policy evidence #363, Intune Windows device: model compliance evaluation and reporting evidence #364, and Intune Windows device: model Windows Update for Business evidence #365 all need these, and Intune Windows device: model compliance evaluation and reporting evidence #364/Intune Windows device: model Windows Update for Business evidence #365 are explicitly told to reuse Intune Windows device: model configuration policy evidence #363's. Leaving them in Intune Windows device: model configuration policy evidence #363 would block the other three behind it.tests/support/is the shared fixture harness, so fifteen leaves do not each copy ~300 lines of manifest/expected/evidence validation.tests/fixtures/intune/_skeleton/pins the reference corpus layout the leaves copy.The CI change matters most
Every Rust step in the
checkjob runs withworking-directory: src-tauri, which selects only thecmtrace-openpackage. The sole parser-crate test in CI was the Windows-only--test esp_diagnostics.New parser test targets would not have run at all, and the parser crate had no clippy gate. Both are now wired into the
checkjob once, so each leaf added under #356 lands gated without editing a workflow.Behavior
Unchanged. No existing module, type, or public path is modified.
intune::modelskeeps its hand-written 13-fieldSerializeimpl untouched.src/lib.rs,Cargo.toml, andCargo.lockare untouched.Verification
The 19 tests in
intune_skeleton_contract.rsinclude 14 adversarial mutations proving the fixture validator actually rejects version drift, byte-count lies, path escapes, duplicate artifact ids, coverage omissions, and uncited findings. A validator that silently passes everything would be worse than none.The second commit fixes a false negative found in review: the privacy scanner tokenized only on whitespace, quotes, and commas, so
a@corp.com;b@corp.comarrived as one token and both real addresses slipped through. A pasted mail header is exactly the shape most likely to carry real customer data into a fixture.Refs #356
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests