feat(intune): parse Company Portal Windows LocalState logs - #389
Closed
adamgell wants to merge 3 commits into
Closed
feat(intune): parse Company Portal Windows LocalState logs#389adamgell wants to merge 3 commits into
adamgell wants to merge 3 commits into
Conversation
Adds `cmtraceopen_parser::intune::portal::windows::company_portal::logs`, a dedicated parser plus canonical evidence document for `%LOCALAPPDATA%\Packages\Microsoft.CompanyPortal_8wekyb3d8bbwe\LocalState\Log_<n>.log` and the sibling `Log.<BridgeName>_<n>.log` bridge logs. Evidence basis and its limitation --------------------------------- Microsoft documents the path and the `Log_<n>.log` pattern but not the record grammar. Exactly ONE verbatim record has ever been published, from Company Portal app version 12-0-0: 2024-11-15T16:50:07.2850341Z INFO Event None 0 <guid> 12-0-0 [Configuration Manager Trace Listener] ... Everything here is derived from that single record, so the grammar is version-scoped from the start: - records are read with GrammarVersion::V1; - 12-0-0 is the only validated app version. Any other version still parses with V1 (it is the only grammar that exists) but downgrades the selection to ParserProvenance::Heuristic and the document to Experimental / Low confidence, and names the gap in coverage; - document confidence never reaches High. Raising it requires a second app version captured from a real device. Encoding, newline style, rotation ordering, the full severity vocabulary, and whether payloads genuinely span lines are all unproven from public evidence. Each is handled defensively rather than assumed, and the open items are recorded in the module docs. Detection safety ---------------- `Log_<n>.log` is a generic name that any UWP package can use, so the file name only nominates a candidate. Confirmation requires field 6 to be a hyphenated GUID and field 7 to be a dash-separated version triple. Two negative fixtures prove it: a column-aligned unrelated UWP log with ISO instants and a severity column is refused even when it sits at the exact Company Portal path, and a generic timestamped log stays on the generic parser. Losslessness and privacy ------------------------ The nested legacy ConfigMgr trace text inside the message — including its day-first date — is never stripped or reinterpreted. Records that fail validation keep their original text and are reported through parse_errors and a coverage row rather than dropped. Dedicated severity wins over keyword inference; only an unrecognized token defers to it. The evidence document is redacted by default and reuses the existing ESP free-text rule table rather than growing a second one; the unredacted form is an explicitly named local-only opt-out. The viewer's LogEntry path is never redacted, because it has to show the file the user opened. Also: the CI parser-crate step now runs every test target in the crate (it named a single target, so new targets ran nowhere), adds a parser-crate clippy gate, and `.gitignore`'s `Logs/` rule is un-ignored for the new `logs/` directories, which it was matching case-insensitively on macOS and Windows checkouts. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdded a Company Portal Windows log parser with version-aware grammar validation, record framing, typed documents, redaction, parser detection, UI integration, documentation, and end-to-end tests. ChangesCompany Portal Windows logs
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a dedicated parser and evidence document pipeline for Windows Company Portal LocalState Log_<n>.log files within the cmtraceopen-parser Intune “portal” family, including detection hardening, redaction, and extensive fixture-backed contract tests; also wires the new parser kind through backend, frontend, docs, and CI.
Changes:
- Implement
cmtraceopen_parser::intune::portal::windows::company_portal::logs(grammar, framing, entries, document, models, redaction) and register detection + parsing routes. - Add a large fixture matrix and integration tests (parser crate + native contract corpus) to ensure conservative detection and lossless parsing behavior.
- Update UI labels/columns, docs, and CI to recognize and validate the new
companyPortalparser.
Reviewed changes
Copilot reviewed 26 out of 45 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/types/log.ts | Add companyPortal to frontend parser kind/implementation unions. |
| src/stores/log-store.ts | Add UI labels for companyPortal parser/implementation. |
| src/lib/column-config.ts | Add default column mapping for companyPortal. |
| src-tauri/tests/parser_supported_formats.rs | Extend native parser format inventory + add Company Portal contract tests. |
| src-tauri/tests/corpus/company_portal/negative/Log_1.log | Add negative native corpus fixture. |
| src-tauri/tests/corpus/company_portal/clean/Log_1.log | Add positive native corpus fixture. |
| src-tauri/src/commands/bundle_ops.rs | Add human-readable parser selection description for Company Portal. |
| references/log-intune-reference.md | Document Company Portal log grammar expectations and version scoping. |
| crates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v13-4-2/unknown-app-version/Log_1.log | Add fixture to exercise unknown-version downgrade behavior. |
| crates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/truncated-boundaries/Log_1.log | Add truncated-boundary fixture coverage. |
| crates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/severity-levels/Log_1.log | Add severity-vocabulary fixture coverage. |
| crates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/same-timestamp-distinct-activity/Log_1.log | Add same-timestamp/different-activity fixture coverage. |
| crates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/rotation/Log_2.log | Add rotation fixture member. |
| crates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/rotation/Log_1.log | Add rotation fixture member. |
| crates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/redaction/Log_1.log | Add redaction fixture coverage. |
| crates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/negative-unrelated-uwp/Log_1.log | Add unrelated-UWP negative fixture. |
| crates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/negative-generic-timestamped/Log_1.log | Add generic-timestamped negative fixture. |
| crates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/multiline-continuation/Log_1.log | Add multiline/continuation framing fixture. |
| crates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/malformed-structural-token/Log_1.log | Add malformed-structure fixture coverage. |
| crates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/invalid-timestamp/Log_1.log | Add invalid-timestamp fixture coverage. |
| crates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/encoding-utf8-nobom/Log_1.log | Add UTF-8 no-BOM fixture coverage. |
| crates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/encoding-utf8-bom/Log_1.log | Add UTF-8 BOM fixture coverage. |
| crates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/code-tokens/Log_1.log | Add code-token fixture coverage. |
| crates/cmtraceopen-parser/tests/company_portal_windows_logs.rs | Add parser-crate integration suite for Company Portal logs. |
| crates/cmtraceopen-parser/src/parser/mod.rs | Route ParserImplementation::CompanyPortal to the new parser. |
| crates/cmtraceopen-parser/src/parser/detect.rs | Add Company Portal detection + heuristic provenance downgrade logic and tests. |
| crates/cmtraceopen-parser/src/models/log_entry.rs | Add CompanyPortal to ParserKind and ParserImplementation enums. |
| crates/cmtraceopen-parser/src/intune/portal/windows/mod.rs | Introduce intune::portal::windows module. |
| crates/cmtraceopen-parser/src/intune/portal/windows/company_portal/mod.rs | Introduce company_portal module under portal/windows. |
| crates/cmtraceopen-parser/src/intune/portal/windows/company_portal/logs/mod.rs | Add Company Portal logs module façade + re-exports. |
| crates/cmtraceopen-parser/src/intune/portal/windows/company_portal/logs/redaction.rs | Implement redacted export projection using shared ESP rule table. |
| crates/cmtraceopen-parser/src/intune/portal/windows/company_portal/logs/models.rs | Add evidence document wire schema types/constants. |
| crates/cmtraceopen-parser/src/intune/portal/windows/company_portal/logs/grammar.rs | Implement strict V1 record grammar (timestamp/GUID/version confirmation). |
| crates/cmtraceopen-parser/src/intune/portal/windows/company_portal/logs/framing.rs | Implement logical-record framing with conservative continuation rules. |
| crates/cmtraceopen-parser/src/intune/portal/windows/company_portal/logs/entries.rs | Map records to LogEntry for viewer rendering. |
| crates/cmtraceopen-parser/src/intune/portal/windows/company_portal/logs/document.rs | Build canonical evidence document + coverage reporting. |
| crates/cmtraceopen-parser/src/intune/portal/windows/company_portal/logs/detect.rs | Implement hint parsing + record-structure confirmation helpers. |
| crates/cmtraceopen-parser/src/intune/portal/mod.rs | Add top-level Company Portal (“portal”) surface module. |
| crates/cmtraceopen-parser/src/intune/mod.rs | Re-export new portal module from intune. |
| crates/cmtraceopen-parser/src/esp/redaction.rs | Make redact_text reusable within the crate for other evidence modules. |
| crates/cmtraceopen-parser/src/esp/mod.rs | Re-export crate-internal redact_text for sibling evidence modules. |
| .gitignore | Unignore new logs/ directories that were being ignored via case-insensitive Logs/. |
| .github/workflows/cmtrace-ci.yml | Run full parser-crate tests + clippy gate in CI. |
| .gitattributes | Mark new fixtures/corpus as byte/whitespace sensitive (-text -whitespace). |
Comment on lines
+117
to
+120
| parse_state: match framed.kind { | ||
| FramedRecordKind::Malformed => CompanyPortalParseState::Malformed, | ||
| _ => CompanyPortalParseState::Orphaned, | ||
| }, |
Owner
Author
There was a problem hiding this comment.
Not a compile error — declining this one.
The inner match framed.kind has two arms, FramedRecordKind::Malformed and _. Neither binds any data: Malformed is a unit variant and _ is a wildcard. Matching a place expression against patterns that bind nothing by value does not move out of the reference, so FramedRecordKind not being Copy is not relevant here. A move would only occur if an arm bound the Box<CompanyPortalRecordFields> payload of the Record variant, and no arm does.
Verified on this branch:
cargo test --locked -p cmtraceopen-parser
lib 403 passed | company_portal_windows_logs 32 passed | esp_diagnostics 222 passed | 0 failed
cargo clippy --locked -p cmtraceopen-parser --all-targets -- -D warnings
clean
cargo check --locked -p cmtraceopen-parser --target wasm32-unknown-unknown
Finished
If it did not compile, none of those would have run.
A code review of this branch produced six findings. None reached the
confidence bar to post as blocking review comments, but four were verified
accurate and are documentation/API-surface defects worth correcting rather
than shipping.
Before: several doc comments claimed more than the implementation delivers.
* models.rs described field 5 as a "monotonic sequence value" while
grammar.rs documented the same field as "semantics unproven". One published
record cannot establish monotonicity, and nothing checks it. The claim is
removed; the field is now described as an unsigned integer of unproven
semantics, which is what the evidence supports.
* CompanyPortalTimestampKind::Invalid was documented as "the field had the
right shape but is not a real instant", but nothing ever constructed it:
parse_utc_instant returns None for that input, so the record is framed
Malformed and reaches the document with timestamp: None. The variant was
dead public API on a published crate describing behavior that does not
happen. Removed, and the enum now documents what actually occurs. Dropping
a half-resolved timestamp is the correct behavior, so only the type and its
doc change.
* matches_company_portal_log_record claimed it was "used by parser::detect".
parser::detect calls classify_line directly, because it needs the
classification to count validated app versions rather than a bool. The
function is the house-convention boolean wrapper; the doc now says so.
* The module claimed losslessness in three places while framing.rs strips
trailing whitespace from every line and drops blank lines entirely. Neither
is reversible from raw_text. The claims are narrowed to what holds — a
record the grammar cannot read is still reported rather than dropped — and
framing.rs now names both exceptions explicitly, including which rule has
to change if a multi-line payload containing a blank line is ever observed.
Why this seam: these are contract statements on a crate published to
crates.io, in a module whose entire premise is not claiming more than the
evidence proves. A doc that overclaims is the same defect class the module
exists to avoid.
Verified on this commit:
cargo test --locked -p cmtraceopen-parser
-> lib 403 passed, company_portal_windows_logs 32 passed,
esp_diagnostics 222 passed, 0 failed
cargo clippy --locked -p cmtraceopen-parser --all-targets -- -D warnings
-> clean
cargo fmt --check --all
-> no diff in any file this branch touches
Refs #366
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
adamgell
added
feature
18 tasks
Owner
Author
|
@copilot Is this ready? |
Contributor
Yes — this is ready from my side. The latest completed CI run for this branch (30608185881) is fully green across Rust, TypeScript, E2E, and all platform builds; only the current "Addressing comment" automation run is still in progress. |
Owner
Author
|
@copilot resolve the merge conflicts on this branch. |
Contributor
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 41 changed files in this pull request and generated no new comments.
Suppressed comments (2)
crates/cmtraceopen-parser/src/parser/detect.rs:658
company_portal_validated_count > 0marks the whole file asDedicatedeven if some confirmed records report an unvalidated app version. That can make parser selection provenance look “validated” in the UI while the evidence document correctly downgrades to Experimental/Low. Consider treating the selection as validated only when all classified Company Portal records are from validated app versions; otherwise selectcompany_portal_experimental().
if company_portal_validated_count > 0 {
ResolvedParser::company_portal()
} else {
ResolvedParser::company_portal_experimental()
}
crates/cmtraceopen-parser/src/intune/portal/windows/company_portal/logs/grammar.rs:288
severity_levelallocates a newStringfor every record viato_ascii_uppercase(). Since this runs on every parsed line, it’s avoidable overhead;eq_ignore_ascii_casekeeps the same behavior without allocating.
fn severity_level(token: &str) -> CompanyPortalSeverityLevel {
match token.to_ascii_uppercase().as_str() {
"INFO" => CompanyPortalSeverityLevel::Information,
"WARN" | "WARNING" => CompanyPortalSeverityLevel::Warning,
"ERROR" => CompanyPortalSeverityLevel::Error,
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/portal/windows/company_portal/logs/grammar.rs`:
- Around line 117-121: Update the comment immediately above the emptiness check
in the record-parsing function to explicitly include field 2 (severity)
alongside fields 3 and 4, so it accurately describes all fields validated by the
condition; leave the check itself unchanged.
In `@crates/cmtraceopen-parser/src/parser/detect.rs`:
- Around line 1012-1020: Add a negative boundary test alongside
test_detect_company_portal_from_content_without_path_hint that passes one
Company Portal record from an unrelated path to detect_parser and asserts the
result is not ParserKind::CompanyPortal, preserving the content-only threshold
of at least two records.
In `@crates/cmtraceopen-parser/tests/company_portal_windows_logs.rs`:
- Around line 496-503: Strengthen the post-projection immutability check in the
test around redacted_export_projection by reasserting the original local input
content, not just local.redacted. Compare the relevant content fields or
serialized input against their pre-projection values after calling
redacted_export_projection, while preserving the existing sensitive-value and
flag assertions.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 73df242f-c8d1-4e07-a53b-1663102c04de
⛔ Files ignored due to path filters (18)
crates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/code-tokens/Log_1.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/encoding-utf16le/Log_1.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/encoding-utf8-bom/Log_1.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/encoding-utf8-nobom/Log_1.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/invalid-timestamp/Log_1.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/malformed-structural-token/Log_1.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/multiline-continuation/Log_1.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/negative-generic-timestamped/Log_1.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/negative-unrelated-uwp/Log_1.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/redaction/Log_1.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/rotation/Log_1.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/rotation/Log_2.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/same-timestamp-distinct-activity/Log_1.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/severity-levels/Log_1.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/truncated-boundaries/Log_1.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v13-4-2/unknown-app-version/Log_1.logis excluded by!**/*.logsrc-tauri/tests/corpus/company_portal/clean/Log_1.logis excluded by!**/*.logsrc-tauri/tests/corpus/company_portal/negative/Log_1.logis excluded by!**/*.log
📒 Files selected for processing (23)
.gitattributes.github/workflows/cmtrace-ci.ymlcrates/cmtraceopen-parser/src/esp/mod.rscrates/cmtraceopen-parser/src/esp/redaction.rscrates/cmtraceopen-parser/src/intune/portal/windows/company_portal/logs/detect.rscrates/cmtraceopen-parser/src/intune/portal/windows/company_portal/logs/document.rscrates/cmtraceopen-parser/src/intune/portal/windows/company_portal/logs/entries.rscrates/cmtraceopen-parser/src/intune/portal/windows/company_portal/logs/framing.rscrates/cmtraceopen-parser/src/intune/portal/windows/company_portal/logs/grammar.rscrates/cmtraceopen-parser/src/intune/portal/windows/company_portal/logs/mod.rscrates/cmtraceopen-parser/src/intune/portal/windows/company_portal/logs/models.rscrates/cmtraceopen-parser/src/intune/portal/windows/company_portal/logs/redaction.rscrates/cmtraceopen-parser/src/intune/portal/windows/company_portal/mod.rscrates/cmtraceopen-parser/src/models/log_entry.rscrates/cmtraceopen-parser/src/parser/detect.rscrates/cmtraceopen-parser/src/parser/mod.rscrates/cmtraceopen-parser/tests/company_portal_windows_logs.rsreferences/log-intune-reference.mdsrc-tauri/src/commands/bundle_ops.rssrc-tauri/tests/parser_supported_formats.rssrc/lib/column-config.tssrc/stores/log-store.tssrc/types/log.ts
Comment on lines
+117
to
+121
| // Fields 3 and 4 carry no validatable shape, but an empty column would mean | ||
| // the split landed somewhere other than a real record boundary. | ||
| if fields[1].is_empty() || fields[2].is_empty() || fields[3].is_empty() { | ||
| return None; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Comment omits field 2 from the field-emptiness check.
The comment at Line 117 says "Fields 3 and 4 carry no validatable shape," naming only category and scenario. The check on Line 119 also tests fields[1] (severity, field 2). Severity has no other shape validation in this function, so the check is correct, but the comment misstates which fields it covers. Update the comment so it matches the code.
📝 Proposed comment fix
- // Fields 3 and 4 carry no validatable shape, but an empty column would mean
- // the split landed somewhere other than a real record boundary.
+ // Fields 2, 3, and 4 (severity, category, scenario) carry no validatable
+ // shape of their own, but an empty column would mean the split landed
+ // somewhere other than a real record boundary.
if fields[1].is_empty() || fields[2].is_empty() || fields[3].is_empty() {
return None;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Fields 3 and 4 carry no validatable shape, but an empty column would mean | |
| // the split landed somewhere other than a real record boundary. | |
| if fields[1].is_empty() || fields[2].is_empty() || fields[3].is_empty() { | |
| return None; | |
| } | |
| // Fields 2, 3, and 4 (severity, category, scenario) carry no validatable | |
| // shape of their own, but an empty column would mean the split landed | |
| // somewhere other than a real record boundary. | |
| if fields[1].is_empty() || fields[2].is_empty() || fields[3].is_empty() { | |
| return None; | |
| } |
🤖 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/portal/windows/company_portal/logs/grammar.rs`
around lines 117 - 121, Update the comment immediately above the emptiness check
in the record-parsing function to explicitly include field 2 (severity)
alongside fields 3 and 4, so it accurately describes all fields validated by the
condition; leave the check itself unchanged.
Comment on lines
+1012
to
+1020
| #[test] | ||
| fn test_detect_company_portal_from_content_without_path_hint() { | ||
| let content = "2024-11-15T16:50:07.2850341Z INFO Event None 0 1487dc30-3bb0-46bf-98ee-76771bd9953e 12-0-0 [Sync] started\n\ | ||
| 2024-11-15T16:50:08.1120000Z INFO Event None 1 1487dc30-3bb0-46bf-98ee-76771bd9953e 12-0-0 [Sync] complete"; | ||
|
|
||
| let detected = detect_parser("C:/Temp/exported-portal-log.txt", content); | ||
|
|
||
| assert_eq!(detected.parser, ParserKind::CompanyPortal); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Add the negative boundary for the content-only threshold.
test_detect_company_portal_from_content_without_path_hint uses two records, so it exercises company_portal_count >= 2. No test pins the other side of that boundary: one Company Portal record with no path hint must not select the parser. That guard is what stops a single stray line in an unrelated file from claiming the format, and a future change to the threshold would pass the suite unnoticed.
Proposed test
#[test]
fn test_single_company_portal_record_without_path_hint_is_not_enough() {
let content = "2024-11-15T16:50:07.2850341Z INFO Event None 0 1487dc30-3bb0-46bf-98ee-76771bd9953e 12-0-0 [Sync] started";
let detected = detect_parser("C:/Temp/unrelated.txt", content);
assert_ne!(detected.parser, ParserKind::CompanyPortal);
}🤖 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/parser/detect.rs` around lines 1012 - 1020, Add
a negative boundary test alongside
test_detect_company_portal_from_content_without_path_hint that passes one
Company Portal record from an unrelated path to detect_parser and asserts the
result is not ParserKind::CompanyPortal, preserving the content-only threshold
of at least two records.
Comment on lines
+496
to
+503
| // And the redacting projection over the same input drops everything. | ||
| let safe = redacted_export_projection(&local); | ||
| let safe_json = serde_json::to_string(&safe).expect("document must serialize"); | ||
| for value in SENSITIVE_VALUES { | ||
| assert!(!safe_json.contains(value), "{value}"); | ||
| } | ||
| assert!(!local.redacted, "projection must not mutate its input"); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Tighten the no-mutation assertion.
Line 502 checks only the redacted flag. A redacted_export_projection that redacted its input in place while leaving the flag false would still pass this test. Re-assert the input content after the projection runs.
Proposed change
// And the redacting projection over the same input drops everything.
let safe = redacted_export_projection(&local);
let safe_json = serde_json::to_string(&safe).expect("document must serialize");
for value in SENSITIVE_VALUES {
assert!(!safe_json.contains(value), "{value}");
}
assert!(!local.redacted, "projection must not mutate its input");
+ let local_json_after = serde_json::to_string(&local).expect("document must serialize");
+ assert_eq!(
+ json, local_json_after,
+ "projection must not mutate its input"
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // And the redacting projection over the same input drops everything. | |
| let safe = redacted_export_projection(&local); | |
| let safe_json = serde_json::to_string(&safe).expect("document must serialize"); | |
| for value in SENSITIVE_VALUES { | |
| assert!(!safe_json.contains(value), "{value}"); | |
| } | |
| assert!(!local.redacted, "projection must not mutate its input"); | |
| } | |
| // And the redacting projection over the same input drops everything. | |
| let safe = redacted_export_projection(&local); | |
| let safe_json = serde_json::to_string(&safe).expect("document must serialize"); | |
| for value in SENSITIVE_VALUES { | |
| assert!(!safe_json.contains(value), "{value}"); | |
| } | |
| assert!(!local.redacted, "projection must not mutate its input"); | |
| let local_json_after = serde_json::to_string(&local).expect("document must serialize"); | |
| assert_eq!( | |
| json, local_json_after, | |
| "projection must not mutate its input" | |
| ); | |
| } |
🤖 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/company_portal_windows_logs.rs` around lines
496 - 503, Strengthen the post-projection immutability check in the test around
redacted_export_projection by reasserting the original local input content, not
just local.redacted. Compare the relevant content fields or serialized input
against their pre-projection values after calling redacted_export_projection,
while preserving the existing sensitive-value and flag assertions.
Owner
Author
|
Superseded by #460, a new clean draft PR from This PR remains a stale recovery artifact: its head |
This was referenced Aug 5, 2026
Merged
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.
Implements the Company Portal Windows
Log_<n>.logparser for #366 (epic #356).Adds
cmtraceopen_parser::intune::portal::windows::company_portal::logs— 7 modules (grammar, detect, framing, entries, document, models, redaction) plus a 695-line integration suite.Detection safety
Log_<n>.logis a name any UWP package can use, so the path only nominates a candidate. Confirmation requires field 6 to be a hyphenated GUID and field 7 to be a dash-separated version triple. The strongest negative fixture places a column-aligned unrelated UWP log — ISO instants, severity column, same file name — at the exact Company Portal LocalState path, and it still resolves toParserKind::Timestamped.Version scoping
12-0-0is the only entry inVALIDATED_APP_VERSIONS. Any other version still parses under the V1 grammar but downgradesParserProvenancetoHeuristicand the document to Experimental/Low, with a named coverage row. Confidence is structurally capped at Medium.Fixture matrix
All 13 items from the issue are present under
tests/fixtures/intune/portal/windows/logs/{v12-0-0,v13-4-2}/, covered by 32 tests. Fixtures are byte-real: 15 of 16 use CRLF, one is LF-only, one is genuine UTF-16LE with an FF FE BOM, one has a UTF-8 BOM. All values are synthetic (sequential placeholder GUIDs, no email addresses).Verified
Run on this exact commit:
Enumeration coverage is complete:
ParserKind/ParserImplementationadded inmodels/log_entry.rs, routed inparser/mod.rs, described insrc-tauri/src/commands/bundle_ops.rs, and mirrored in all three frontend sites (src/types/log.ts,src/stores/log-store.ts,src/lib/column-config.ts).Known gaps — deliberately not claimed as done
Stating these plainly rather than letting review discover them:
12-0-0, the single record Microsoft published) backs the grammar. Thev13-4-2fixture is invented to exercise the downgrade path, not a captured second-version sample. Windows build, file ordering, and rollover behavior are recorded as UNKNOWN rather than established from evidence. The issue asks for samples from at least two versions "when available"; a second was not available.document.rsdeliberately declines semantic phase classification. The issue scopes this to "proven concepts" only, and none are proven from one sample — but it means zero normalized evidence ships here.CompanyPortalLogRecordhas no dedicated code-token field; codes survive insidemessage/raw_text, and theLogEntrypath getserror_code_spansfrom the pre-existing shared pipeline.CompanyPortalLogDocumenthas noencodingfield. Encoding is detected upstream byparser::detect_encodingand discarded before the document is built, so a document cannot report the encoding of its source. This misses part of the shared "preserve ... encoding ..." constraint.Items 2 and 4 are small and could land here on request; item 1 needs a real second-version sample.
Incidental fixes bundled in
.github/workflows/cmtrace-ci.ymlran only--test esp_diagnosticsfor the parser crate, so any new test target ran nowhere. Now runs the whole crate, plus a parser-crate clippy gate..gitignore'sLogs/rule was matching the newlogs/directories case-insensitively on macOS/Windows checkouts; re-included..gitattributesmarks the byte-sensitive fixtures-text -whitespace.Refs #366
Summary by CodeRabbit
New Features
Documentation
Bug Fixes