fix(intune): validate AppId GUID identities - #433
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Warning
|
| Layer / File(s) | Summary |
|---|---|
GUID field validation crates/cmtraceopen-parser/src/intune/guid_registry.rs |
Explicit identity fields now use shared classification, canonical validation, object scoping, duplicate handling, and normalized registry lookups. |
Identity extraction integration crates/cmtraceopen-parser/src/intune/download_stats.rs, crates/cmtraceopen-parser/src/intune/event_tracker.rs |
Download and AppWorkload extraction now prioritize valid explicit identities, suppress fallback for invalid or conflicting fields, and retain heuristics when fields are absent. |
Safe sidecar identifier handling crates/cmtraceopen-parser/src/intune/event_tracker.rs |
Sidecar script prefixes now use character counts without splitting multibyte values. Tests cover identity precedence, suppression, fallback, and UTF-8 values. |
Estimated code review effort: 4 (Complex) | ~45 minutes
Sequence Diagram(s)
sequenceDiagram
participant IntuneMessage
participant GuidRegistry
participant ParserConsumer
IntuneMessage->>GuidRegistry: parse AppId and Id fields
GuidRegistry->>GuidRegistry: classify and validate identity
GuidRegistry-->>ParserConsumer: explicit identity or fallback state
ParserConsumer->>ParserConsumer: attribute content or workload GUID
Possibly related PRs
- adamgell/cmtraceopen#388: Introduced the Intune parser structure extended by this change.
Suggested labels: test
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
| Check name | Status | Explanation |
|---|---|---|
| Docstring Coverage | ✅ Passed | Docstring coverage is 86.96% which is sufficient. The required threshold is 80.00%. |
| Linked Issues check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title clearly and concisely describes the main change: validating Intune AppId GUID identities. |
✨ Finishing Touches
📝 Generate docstrings
- Create stacked PR
- Commit on current branch
Comment @coderabbitai help to get the list of available commands.
|
@coderabbitai review @github-copilot review Please review the independently reconstructed AppId identity-validation and Unicode-safe display-boundary change. In particular, verify that rejecting non-GUID AppId values preserves the existing GUID-only correlation contract. |
github-actions
Bot
added
bug
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '--- committed diff summary ---'
git diff --stat HEAD^ HEAD
echo
echo '--- changed files ---'
git diff --name-only HEAD^ HEAD
echo
echo '--- diff ---'
git diff --find-renames --find-copies --unified=80 HEAD^ HEAD -- \
':!Cargo.lock'
echo
echo '--- Rust source outline for changed files ---'
git diff --name-only HEAD^ HEAD | while IFS= read -r file; do
case "$file" in
*.rs)
echo "### $file"
ast-grep outline "$file" --items all --view compact || true
;;
esac
doneLength of output: 23174 🏁 Script executed: #!/bin/bash
set -euo pipefail
registry='crates/cmtraceopen-parser/src/intune/guid_registry.rs'
tracker='crates/cmtraceopen-parser/src/intune/event_tracker.rs'
echo '--- source maps ---'
ast-grep outline "$registry" --items all --view signatures
ast-grep outline "$tracker" --items all --view signatures
echo
echo '--- identity extraction declarations and regex definitions ---'
rg -n -C 12 \
'fn (extract_app_id|extract_guid_field|extract_guid|has_name_field|extract_json_field)|fn (guid_re|app_id_json_re)\b|static .*GUID|Lazy.*guid' \
crates/cmtraceopen-parser/src/intune
echo
echo '--- GuidRegistry ingestion and identity extraction calls ---'
rg -n -C 10 \
'\b(fn ingest_lines|extract_app_id\s*\(|extract_all_id_name_pairs\s*\(|extract_guid\s*\()' \
crates/cmtraceopen-parser/src/intune
echo
echo '--- identity-related tests and named-context assertions ---'
rg -n -C 8 \
'(extract_app_id|extract_all_id_name_pairs|GuidRegistry::new|has_name_field|ApplicationName|SetUpFilePath|non_guid|invalid.*guid|guid.*fallback)' \
"$registry" "$tracker"Length of output: 102681 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '--- event GUID extraction implementation ---'
sed -n '1138,1174p' crates/cmtraceopen-parser/src/intune/event_tracker.rs
echo
echo '--- registry identity extraction implementation ---'
sed -n '92,120p' crates/cmtraceopen-parser/src/intune/guid_registry.rs
sed -n '279,334p' crates/cmtraceopen-parser/src/intune/guid_registry.rs
echo
echo '--- all shared extractor imports and calls ---'
rg -n -C 4 \
'(use .*extract_app_id|extract_app_id\s*\()' \
crates/cmtraceopen-parser/src
echo
echo '--- download-specific fallback implementation ---'
sed -n '320,370p' crates/cmtraceopen-parser/src/intune/download_stats.rs
echo
echo '--- committed whitespace check ---'
git diff --check HEAD^ HEADLength of output: 15810
The existing named-context fallback remains unchanged. It still permits a GUID found in a message only when a name-bearing field is present. This preserves the compatibility contract without accepting a non-GUID The sidecar display prefix now uses
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/cmtraceopen-parser/src/intune/event_tracker.rs (1)
2037-2053: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise the changed UTF-8 branch directly.
This test proves that the multibyte
AppIddoes not become an event GUID. It does not executeg.get(..8), because the test asserts thatguidisNone. A regression in the slicing expression could therefore pass this test.Extract the shortening operation into a small helper and test it with a value such as
aaaaaa你好.Proposed testable helper
+fn sidecar_identifier_prefix(value: &str) -> &str { + value.get(..8).unwrap_or(value) +} + - .map(|g| g.get(..8).unwrap_or(g)) + .map(sidecar_identifier_prefix) + + #[test] + fn sidecar_identifier_prefix_handles_multibyte_values() { + assert_eq!(sidecar_identifier_prefix("aaaaaa你好"), "aaaaaa你好"); + }🤖 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/event_tracker.rs` around lines 2037 - 2053, Extract the AppId shortening logic from the event parsing path into a small helper, using the same UTF-8-safe behavior as the current inline operation. Add a focused unit test for the helper with “aaaaaa你好” that directly exercises the shortening branch and verifies the expected result without relying on GUID extraction.
🤖 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/guid_registry.rs`:
- Around line 285-295: Update the later app_id_json_re() fallback to validate
its captured value with guid_re() before returning it, matching the validation
used by extract_guid_field and preventing malformed hyphen placements from being
accepted. Add a regression test covering an invalid 36-character GUID with
incorrect hyphen positions, while preserving valid AppId extraction.
---
Nitpick comments:
In `@crates/cmtraceopen-parser/src/intune/event_tracker.rs`:
- Around line 2037-2053: Extract the AppId shortening logic from the event
parsing path into a small helper, using the same UTF-8-safe behavior as the
current inline operation. Add a focused unit test for the helper with “aaaaaa你好”
that directly exercises the shortening branch and verifies the expected result
without relying on GUID extraction.
🪄 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: 75cf86d7-aedc-4768-ac25-7a63d8e38984
📒 Files selected for processing (2)
crates/cmtraceopen-parser/src/intune/event_tracker.rscrates/cmtraceopen-parser/src/intune/guid_registry.rs
|
@coderabbitai review @github-copilot review Fix round 1 is at exact head |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
commit='b0aec8d6b9786ac81b1384ba207149b92fd97f02'
parent="${commit}^"
echo '--- requested commit ---'
git show -s --format='commit=%H%nparent=%P%nsubject=%s' "$commit"
echo
echo '--- changed files and diff ---'
git diff --stat "$parent" "$commit"
git diff --find-renames --find-copies --unified=100 "$parent" "$commit" -- ':!Cargo.lock'
echo
echo '--- Rust outlines for changed files ---'
git diff --name-only "$parent" "$commit" | while IFS= read -r file; do
case "$file" in
*.rs)
echo "### $file"
ast-grep outline "$file" --items all --view signatures
;;
esac
done
echo
echo '--- AppId identity extraction and fallback call sites ---'
rg -n -C 10 \
'extract_(app_id|guid_field|guid)\s*\(|AppId|app_id|has_name_field|extract_all_id_name_pairs' \
crates/cmtraceopen-parser/src/intune
echo
echo '--- prefix helper declarations, call sites, and tests ---'
rg -n -C 10 \
'(prefix|sidecar|Script|script).*|fn [A-Za-z0-9_]*prefix[A-Za-z0-9_]*\s*\(' \
crates/cmtraceopen-parser/src/intune/event_tracker.rs
echo
echo '--- whitespace check ---'
git diff --check "$parent" "$commit"Length of output: 116105
The named-context fallback remains separate. It can return a complete GUID found elsewhere in a message only when that message has a name-bearing field. This preserves the GUID-only correlation contract.
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Pull request overview
This PR hardens Intune log parsing so only canonical GUIDs from AppId / Id JSON fields can become identities used for GUID registry correlation, and it removes a UTF-8 slicing panic in sidecar script event naming.
Changes:
- Tighten
AppIdextraction to accept only canonical8-4-4-4-12GUID shapes (including for direct/escaped JSON fields) while keeping the “named-context” generic GUID fallback. - Replace byte-slicing on potentially non-ASCII values with a UTF-8-safe prefix helper and add focused regression tests for both behaviors.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| crates/cmtraceopen-parser/src/intune/guid_registry.rs | Makes AppId/Id identity extraction GUID-only (canonical shape) and adds regression tests to prevent non-GUID identities from entering the GuidRegistry. |
| crates/cmtraceopen-parser/src/intune/event_tracker.rs | Avoids UTF-8 panics in sidecar event short-name generation via utf8_prefix, and adds tests ensuring non-GUID multibyte AppIds don’t become identities. |
|
Exact-head review requested for This round adds tri-state explicit AppId/Id handling so malformed explicit identities cannot fall through to unrelated line-wide GUIDs in the registry, AppWorkload events, or download correlation. Focused RED tests were captured before implementation. Parser tests, strict Clippy, Rust 1.88 wasm32 check, rustfmt, and diff check are green. @coderabbitai review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
commit='d8c1139eeaeb8f2fb4679569742d8d1daf5ac4ad'
parent="${commit}^"
echo '--- requested commit ---'
git show -s --format='commit=%H%nparent=%P%nsubject=%s' "$commit"
echo
echo '--- changed files ---'
git diff --name-status "$parent" "$commit"
echo
echo '--- full commit diff ---'
git diff --find-renames --find-copies --unified=100 "$parent" "$commit" -- ':!Cargo.lock'
echo
echo '--- Rust outlines for changed files ---'
git diff --name-only "$parent" "$commit" | while IFS= read -r file; do
case "$file" in
*.rs)
echo "### $file"
ast-grep outline "$file" --items all --view signatures
;;
esac
done
echo
echo '--- identity extractors, result variants, and call sites ---'
rg -n -C 12 \
'(enum .*AppId|enum .*Identity|Explicit|extract_(app_id|guid_field|guid)\s*\(|extract_all_id_name_pairs\s*\(|AppId|ApplicationName|SetUpFilePath)' \
crates/cmtraceopen-parser/src/intune
echo
echo '--- identity-related regression tests ---'
rg -n -C 10 \
'(malformed|invalid|non_guid|unrelated|fallback|AppWorkload|download|explicit.*AppId|explicit.*Id)' \
crates/cmtraceopen-parser/src/intune
echo
echo '--- exact-commit whitespace check ---'
git diff --check "$parent" "$commit"Length of output: 201658 🏁 Script executed: #!/bin/bash
set -euo pipefail
commit='d8c1139eeaeb8f2fb4679569742d8d1daf5ac4ad'
parent="${commit}^"
echo '--- exact diff for identity-related files ---'
git diff --unified=40 "$parent" "$commit" -- \
crates/cmtraceopen-parser/src/intune/guid_registry.rs \
crates/cmtraceopen-parser/src/intune/event_tracker.rs \
crates/cmtraceopen-parser/src/intune/download_stats.rs
echo
echo '--- explicit identity classifier and legacy extractor ---'
sed -n '270,390p' crates/cmtraceopen-parser/src/intune/guid_registry.rs
echo
echo '--- registry ingestion identity-selection branch ---'
rg -n -C 30 'fn ingest_lines|explicit_app_identity|extract_app_id\s*\(' \
crates/cmtraceopen-parser/src/intune/guid_registry.rs
echo
echo '--- download identity-selection branch ---'
rg -n -C 25 'fn extract_content_id|explicit_app_identity|content_id_re' \
crates/cmtraceopen-parser/src/intune/download_stats.rs
echo
echo '--- AppWorkload identity-selection branch and focused tests ---'
sed -n '570,610p' crates/cmtraceopen-parser/src/intune/event_tracker.rs
sed -n '2068,2165p' crates/cmtraceopen-parser/src/intune/event_tracker.rs
echo
echo '--- all explicit classifier consumers ---'
rg -n -C 5 '\bexplicit_app_identity\s*\(' crates/cmtraceopen-parser/srcLength of output: 67425
For an invalid For a valid explicit field, the explicit GUID takes precedence over unrelated GUIDs. For no explicit field, the existing named-context and AppWorkload fallback behavior remains available. The regression tests cover direct and escaped fields, whitespace, invalid fields, valid fields, decorated identities, and the retained named-context fallback. The UTF-8 prefix tests remain direct and boundary-safe.
✅ Action performedReview finished.
|
|
The pull request is now ready for review so the hosted bot can process exact head @coderabbitai review |
|
✅ Action performedReview finished.
|
|
Exact-head review requested for This separate round-three commit fixes AppId-over-Id precedence in registry ingestion, AppWorkload events, and download correlation. Focused tests were captured RED on the prior head and are GREEN across direct, escaped, spaced, decorated, and reversed-order forms. Full parser, strict Clippy, Rust 1.88 wasm32, scoped rustfmt, and diff checks pass. Local CodeRabbit exact-range review completed with zero findings. @coderabbitai review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
commit='3920f7dd6462c79033aff8fd6046bd2067a6f575'
parent="${commit}^"
echo '--- exact commit ---'
git show -s --format='commit=%H%nparent=%P%nsubject=%s' "$commit"
echo
echo '--- changed files ---'
git diff --name-status "$parent" "$commit"
echo
echo '--- exact diff ---'
git diff --find-renames --find-copies --unified=80 "$parent" "$commit" -- ':!Cargo.lock'
echo
echo '--- outlines for changed Rust files ---'
git diff --name-only "$parent" "$commit" | while IFS= read -r file; do
case "$file" in
*.rs)
echo "### $file"
ast-grep outline "$file" --items all --view signatures
;;
esac
done
echo
echo '--- classifier, consumers, and tests ---'
rg -n -C 14 \
'\b(ExplicitAppIdentity|explicit_app_identity|extract_app_id|extract_guid_field|extract_content_id|extract_all_id_name_pairs|AppId|Id)\b' \
crates/cmtraceopen-parser/src/intune/guid_registry.rs \
crates/cmtraceopen-parser/src/intune/event_tracker.rs \
crates/cmtraceopen-parser/src/intune/download_stats.rs
echo
echo '--- exact-commit whitespace check ---'
git diff --check "$parent" "$commit"Length of output: 149006
An invalid The added tests cover direct, escaped, whitespace-tolerant, decorated, and reversed-order fields. ✅ Action performedReview finished.
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (1)
crates/cmtraceopen-parser/src/intune/guid_registry.rs:363
- The comment says this is a "whitespace-tolerant escaped AppId" path, but
app_id_json_re()matches the unescaped JSON form ("AppId"in the regex matches a literal"quote, not a backslash+quote sequence). This is misleading when reasoning about what syntaxes are actually covered here; the escaped whitespace-tolerant handling is provided byscan_identity_fieldsinstead.
// Preserve the whitespace-tolerant escaped AppId path with its canonical
// GUID grammar, then scan all direct/escaped forms including decorated
// field-local GUIDs.
|
@coderabbitai review Exact-head review requested for |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
commit='bd440db052613c620680e2bacb90ba0d4776a1f3'
parent="${commit}^"
echo '--- exact commit ---'
git show -s --format='commit=%H%nparent=%P%nsubject=%s' "$commit"
echo
echo '--- changed files ---'
git diff --name-status "$parent" "$commit"
echo
echo '--- exact diff ---'
git diff --find-renames --find-copies --unified=100 "$parent" "$commit" -- ':!Cargo.lock'
echo
echo '--- Rust outlines for changed source files ---'
git diff --name-only "$parent" "$commit" | while IFS= read -r file; do
case "$file" in
*.rs)
echo "### $file"
ast-grep outline "$file" --items all --view signatures
;;
esac
done
echo
echo '--- classifier, identity consumers, and relevant tests ---'
rg -n -C 12 \
'\b(ExplicitAppIdentity|explicit_app_identity|extract_app_id|extract_guid_field|extract_content_id|extract_all_id_name_pairs|AppId|Id|duplicate|escaped|case.insensitive|object.scope)\b' \
crates/cmtraceopen-parser/src/intune/guid_registry.rs \
crates/cmtraceopen-parser/src/intune/event_tracker.rs \
crates/cmtraceopen-parser/src/intune/download_stats.rs
echo
echo '--- exact-commit whitespace check ---'
git diff --check "$parent" "$commit"Length of output: 185716
For example, this line has no duplicate field in either object: The depth-grouping loop classifies both Classify each
You are interacting with an AI system. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
crates/cmtraceopen-parser/src/intune/guid_registry.rs (2)
1230-1238: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert the exact resolved names to make the object-boundary claim verifiable.
The test states that independent objects keep their own
Id/Namepairs. The assertions forapp_guidandfirst_idonly checkis_some(). In the first payload both names are the literal"Shared Name", so a cross-object name mix-up still passes. Assert the expected name for each GUID instead.♻️ Proposed test tightening
- assert!( - registry.resolve(app_guid).is_some(), - "missing AppId mapping" - ); - assert!( - registry.resolve(first_id).is_some(), - "missing first Id mapping" - ); + assert!( + registry.resolve(app_guid).is_some(), + "missing AppId mapping for {message}" + ); + assert!( + registry.resolve(first_id).is_some(), + "missing first Id mapping for {message}" + );Prefer a per-payload expected-name table so each GUID is compared against its own name, and give the two objects distinct names in the first payload.
🤖 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/guid_registry.rs` around lines 1230 - 1238, Strengthen the assertions in the registry test by comparing app_guid and first_id against their exact expected names instead of only checking is_some(). Update the first payload’s two objects to use distinct names, and preferably validate each GUID/name pair through a per-payload expected-name table while preserving the existing second_id assertion.
309-323: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider tracking the backslash run count incrementally.
preceding_backslash_countscans backward from every quote byte. A line that contains a long run of backslashes makes the scan quadratic in the line length. Log lines are normally short, so this is not a practical problem today. A running counter removes the backward scan and keeps the state machine single pass.♻️ Proposed refactor
let bytes = msg.as_bytes(); let mut quote_style = None; let mut stack: Vec<ObjectFrame> = Vec::new(); let mut scopes = Vec::new(); + let mut backslashes = 0usize; for (index, byte) in bytes.iter().copied().enumerate() { + if byte == b'\\' { + backslashes += 1; + continue; + } if byte == b'"' { - let backslashes = preceding_backslash_count(bytes, index); match (quote_style, backslashes) { (Some(QuoteStyle::Direct), count) if count % 2 == 0 => quote_style = None, (Some(QuoteStyle::BackslashEscaped), 1) => quote_style = None, (None, count) if count % 2 == 0 => quote_style = Some(QuoteStyle::Direct), (None, 1) => quote_style = Some(QuoteStyle::BackslashEscaped), _ => {} } + backslashes = 0; continue; } + backslashes = 0;Remove
preceding_backslash_countafter this change, and keep theindexvalue semantics unchanged for{and}.Also applies to: 377-383
🤖 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/guid_registry.rs` around lines 309 - 323, Refactor the quote-tracking loop to maintain a running count of consecutive backslashes instead of calling preceding_backslash_count for each quote. Update the counter as bytes are processed and use its value for the existing QuoteStyle transitions, resetting it appropriately after non-backslash bytes while preserving the current index semantics for “{” and “}”. Remove preceding_backslash_count once unused.
🤖 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/guid_registry.rs`:
- Around line 453-479: Update classify_identity_fields to scan parsed JSON field
boundaries rather than using remaining.find(key), ensuring identity keys inside
string values are ignored and escaped quotes are handled correctly. Preserve
direct and decorated GUID extraction for actual fields, and add a regression
test covering an escaped quote containing a key-like fragment that must not
produce Conflict.
---
Nitpick comments:
In `@crates/cmtraceopen-parser/src/intune/guid_registry.rs`:
- Around line 1230-1238: Strengthen the assertions in the registry test by
comparing app_guid and first_id against their exact expected names instead of
only checking is_some(). Update the first payload’s two objects to use distinct
names, and preferably validate each GUID/name pair through a per-payload
expected-name table while preserving the existing second_id assertion.
- Around line 309-323: Refactor the quote-tracking loop to maintain a running
count of consecutive backslashes instead of calling preceding_backslash_count
for each quote. Update the counter as bytes are processed and use its value for
the existing QuoteStyle transitions, resetting it appropriately after
non-backslash bytes while preserving the current index semantics for “{” and
“}”. Remove preceding_backslash_count once unused.
🪄 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: 85399637-5155-4334-890a-ff182d7cd3a1
📒 Files selected for processing (3)
crates/cmtraceopen-parser/src/intune/download_stats.rscrates/cmtraceopen-parser/src/intune/event_tracker.rscrates/cmtraceopen-parser/src/intune/guid_registry.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/cmtraceopen-parser/src/intune/event_tracker.rs
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (4)
crates/cmtraceopen-parser/src/intune/guid_registry.rs:159
resolvecurrently allocates a lowercasedStringfor every lookup (normalize_guid_key(guid)), even when the caller already passes a normalized key. Since the map key type isString(borrowed as&str), you can fast-pathself.entries.get(guid)and only allocate when needed.
pub fn resolve(&self, guid: &str) -> Option<&str> {
self.entries
.get(&normalize_guid_key(guid))
.map(|entry| entry.name.as_str())
}
crates/cmtraceopen-parser/src/intune/download_stats.rs:366
extract_content_idnormalizes GUIDs fromexplicit_app_identity/extract_app_id, but thecontent_id_re()fallback returns the capture verbatim. If logs emit the same GUID in different case across lines, this can splitactive: HashMap<String, PartialDownload>into multiple entries and produce incorrect download aggregation. Consider lowercasing the regex capture for consistency.
extract_app_id(msg).or_else(|| {
content_id_re()
.captures(msg)
.and_then(|captures| captures.get(1))
.map(|value| value.as_str().to_string())
crates/cmtraceopen-parser/src/intune/guid_registry.rs:139
insert_if_dominatedlowercases GUID keys by allocating a newStringvianormalize_guid_key(&guid), immediately dropping the original. Since the parameter is already an ownedString, this can be done in-place withmake_ascii_lowercase()to avoid an extra allocation per insert/merge.
This issue also appears on line 155 of the same file.
fn insert_if_dominated(&mut self, guid: String, name: String, source: GuidNameSource) {
let guid = normalize_guid_key(&guid);
let dominated = self
.entries
.get(&guid)
crates/cmtraceopen-parser/src/intune/download_stats.rs:362
- In the
ExplicitAppIdentity::Absentbranch, this callsextract_app_id(msg), which re-runsexplicit_app_identity(msg)internally. That means some lines will be scanned twice for explicit identity/object scopes. If this is on a hot path, consider refactoringguid_registryto expose anextract_app_idvariant that accepts a precomputedExplicitAppIdentity(or an internal helper for the fallback-only path) to avoid duplicate work.
// Preserve named-context and download-specific heuristics only
// when the line has no explicit JSON identity field.
extract_app_id(msg).or_else(|| {
content_id_re()
|
@coderabbitai review Exact-head review requested for |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
commit='bf75f08646fa06ed6250c0b277ffc0b8258827c4'
parent="${commit}^"
echo '--- commit ---'
git show -s --format='commit=%H%nparent=%P%nsubject=%s' "$commit"
echo
echo '--- changed files ---'
git diff --name-status "$parent" "$commit"
echo
echo '--- diff ---'
git diff --find-renames --find-copies --unified=100 "$parent" "$commit" -- ':!Cargo.lock'
echo
echo '--- changed Rust outlines ---'
git diff --name-only "$parent" "$commit" | while IFS= read -r file; do
case "$file" in
*.rs)
echo "### $file"
ast-grep outline "$file" --items all --view signatures
;;
esac
done
echo
echo '--- scope parsing, classifier, consumers, and tests ---'
rg -n -C 16 \
'\b(JsonObjectScope|ExplicitAppIdentity|IdentityFieldState|explicit_app_identity|extract_app_id|extract_guid_field|extract_content_id|extract_all_id_name_pairs|AppId|ApplicationName|SetUpFilePath|quoted|sibling|nested|containment|span)\b' \
crates/cmtraceopen-parser/src/intune/guid_registry.rs \
crates/cmtraceopen-parser/src/intune/event_tracker.rs \
crates/cmtraceopen-parser/src/intune/download_stats.rs
echo
echo '--- whitespace check ---'
git diff --check "$parent" "$commit"Length of output: 201294 🏁 Script executed: #!/bin/bash
set -euo pipefail
commit='bf75f08646fa06ed6250c0b277ffc0b8258827c4'
parent="${commit}^"
echo '--- exact changed hunks: guid registry ---'
git diff --unified=60 "$parent" "$commit" -- \
crates/cmtraceopen-parser/src/intune/guid_registry.rs
echo
echo '--- scanner implementation ---'
sed -n '313,530p' crates/cmtraceopen-parser/src/intune/guid_registry.rs
echo
echo '--- registry ingestion implementation ---'
sed -n '105,240p' crates/cmtraceopen-parser/src/intune/guid_registry.rs
echo
echo '--- explicit identity selection ---'
sed -n '578,731p' crates/cmtraceopen-parser/src/intune/guid_registry.rs
echo
echo '--- primary event identity path and generic GUID extractor ---'
sed -n '471,620p' crates/cmtraceopen-parser/src/intune/event_tracker.rs
sed -n '1145,1176p' crates/cmtraceopen-parser/src/intune/event_tracker.rs
echo
echo '--- focused new tests ---'
sed -n '1450,1700p' crates/cmtraceopen-parser/src/intune/guid_registry.rs
sed -n '860,915p' crates/cmtraceopen-parser/src/intune/download_stats.rs
sed -n '2200,2265p' crates/cmtraceopen-parser/src/intune/event_tracker.rsLength of output: 84983 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '--- source classification and ESP detection ---'
sed -n '890,1065p' crates/cmtraceopen-parser/src/intune/event_tracker.rs
sed -n '1170,1325p' crates/cmtraceopen-parser/src/intune/event_tracker.rs
echo
echo '--- ESP references and tests ---'
rg -n -C 12 \
'\bESP\b|esp_re|Enrollment Status|extract_events\s*\(' \
crates/cmtraceopen-parser/src/intune/event_tracker.rs \
crates/cmtraceopen-parser/src/intune
echo
echo '--- all non-AppWorkload event source classifications ---'
rg -n -C 8 \
'enum ImeSourceKind|fn classify_source_kind|ImeSourceKind::' \
crates/cmtraceopen-parser/src/intune/event_tracker.rsLength of output: 153565
As a result, a malformed explicit identity does not suppress an unrelated GUID for ESP events. For example: The ESP event receives Classify
You are interacting with an AI system. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (2)
crates/cmtraceopen-parser/src/intune/guid_registry.rs:167
resolve()always allocates a normalized GUID (normalize_guid_key) even when the input is already lowercase and/or when no matching entry exists. Sinceresolve()is used inunresolved_guids_from()and event-name enrichment, this can add unnecessary allocations on hot paths; consider only allocating when the input contains ASCII uppercase characters.
pub fn resolve(&self, guid: &str) -> Option<&str> {
self.entries
.get(&normalize_guid_key(guid))
.map(|entry| entry.name.as_str())
crates/cmtraceopen-parser/src/intune/guid_registry.rs:147
insert_if_dominatedlowercases GUID keys by allocating a newString(normalize_guid_key(&guid)), even though the function already owns theString. This adds avoidable allocations on every insert/merge; you can lowercase the owned string in-place withmake_ascii_lowercase().
This issue also appears on line 164 of the same file.
/// Insert an entry if no higher-confidence entry already exists for this GUID.
fn insert_if_dominated(&mut self, guid: String, name: String, source: GuidNameSource) {
let guid = normalize_guid_key(&guid);
let dominated = self
.entries
|
Post-merge audit found two additional name-scope defects that broad CI and the first review cycle did not expose:
A separate test-first follow-up is now under review in #438 at exact head |
Recovery scope
Independent reconstruction and review hardening of the Intune batch-two AppId parser work. This branch was rebuilt from
origin/main; it does not copy or modify Claude's worktree.Behavior
AppId/Ididentity fields only when they contain canonical 8-4-4-4-12 GUIDs.AppIdprecedence overIdinside one logical object.Parser input is untrusted log text and registry keys feed later correlation, so ambiguous explicit identities fail closed.
Test-first evidence
Focused failures were captured before each correction, including:
Verification at
bf75f08646fa06ed6250c0b277ffc0b8258827c4cargo test --locked -p cmtraceopen-parser: PASS (515 unit, 26 macOS portal, 27 Windows package-state, 222 ESP, 26 skeleton, 25 remediation, 24 scripts, 1 doc).cargo clippy --locked -p cmtraceopen-parser --all-targets -- -D warnings: PASS.cargo +1.88.0 check --locked -p cmtraceopen-parser --target wasm32-unknown-unknown: PASS.rustfmt --edition 2021 --checkandgit diff --check: PASS.bd440db052613c620680e2bacb90ba0d4776a1f3..bf75f08646fa06ed6250c0b277ffc0b8258827c4: first pass found two valid issues; each received a focused RED regression and fix. Second exact-range pass: PASS, 0 findings across all three changed files.Review state
The current exact head is pushed. Fresh hosted CodeRabbit and GitHub Copilot reviews are requested for this head. Hosted CI is running; approvals on older commits are not treated as current acceptance.