feat(intune): deterministic Company Portal Windows package-state evidence - #395
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThe PR adds a versioned Windows Company Portal package-state contract. It replaces formatted collector output with JSON, adds parsing and deterministic findings, supports legacy imports, provides redacted exports, and adds comprehensive fixtures and tests. ChangesCompany Portal package-state evidence
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant AppXAdapter
participant PackageStateParser
participant FindingDeriver
participant RedactedExport
AppXAdapter->>PackageStateParser: emit and parse versioned JSON capture
PackageStateParser->>FindingDeriver: provide PackageStateCapture
FindingDeriver-->>PackageStateParser: return deterministic findings
PackageStateParser->>RedactedExport: provide parsed capture
RedactedExport-->>PackageStateParser: return redacted projection
Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
adamgell
added
feature
Contributor
There was a problem hiding this comment.
Pull request overview
Introduces a deterministic, versioned JSON capture contract and corresponding findings/redaction logic for Windows Company Portal (and Authenticator) AppX package-state evidence under cmtraceopen_parser::intune::portal, replacing reliance on locale-/format-dependent PowerShell Format-List output.
Changes:
- Adds
intune::portal::windows::company_portal::package_stateschema + findings + redaction, including a strict legacyFormat-Listimport path that can refuse unsafe input. - Updates the collector’s embedded Windows command profile to emit the JSON envelope via
ConvertTo-Json, plus adds a Windows-only native test to validate the adapter output parses. - Adds a focused contract test suite and a 14-scenario fixture matrix (including determinism and privacy redaction).
Reviewed changes
Copilot reviewed 33 out of 33 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src-tauri/src/collector/mod.rs | Adds Windows-only test that runs the embedded AppX adapter and validates schema parseability. |
| crates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/package_state/unknown-future-schema/capture.json | Fixture for forward-schema handling (unsupported schema + raw preservation). |
| crates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/package_state/privacy-paths/golden-redacted.json | Golden output for redacted export projection. |
| crates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/package_state/privacy-paths/capture.json | Fixture containing identity/path data to validate redaction. |
| crates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/package_state/per-user-only-registration/capture.json | Fixture for current-user-only registration + all-users not queried coverage. |
| crates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/package_state/package-status-problem/capture.json | Fixture for non-OK package status producing an error finding. |
| crates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/package_state/multiple-registrations/capture.json | Fixture for duplicate registrations of the same package family. |
| crates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/package_state/malformed-json/capture.json | Fixture validating malformed JSON becomes a typed error + finding (not panic). |
| crates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/package_state/legacy-format-list-refused/wrapped.txt | Legacy sample that must refuse due to wrapped/truncated records. |
| crates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/package_state/legacy-format-list-refused/non-english.txt | Legacy sample that must refuse due to non-English localization. |
| crates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/package_state/legacy-format-list-english/packages.txt | Legacy English Format-List sample that imports as low-confidence evidence. |
| crates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/package_state/installed-company-portal/capture.json | Fixture for installed Company Portal row and installed finding. |
| crates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/package_state/installed-company-portal-and-authenticator/capture.json | Fixture for separate Company Portal + Authenticator rows. |
| crates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/package_state/deterministic-serialization/golden.json | Golden bytes for deterministic serialization contract. |
| crates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/package_state/deterministic-serialization/capture.json | Input permutation fixture for field-order-independent serialization. |
| crates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/package_state/command-failure/capture.json | Fixture for failed command status producing coverage findings (no absence claims). |
| crates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/package_state/access-denied-incomplete-query/capture.json | Fixture for access-denied capture and denied scope coverage. |
| crates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/package_state/absent-after-complete-all-users-capture/capture.json | Fixture ensuring absence is only claimable with completed command + complete scope coverage. |
| crates/cmtraceopen-parser/tests/company_portal_windows_package_state.rs | Contract tests covering the schema, findings, determinism, legacy import, and redaction. |
| crates/cmtraceopen-parser/src/wire.rs | Adds shared raw_preserving_string_enum! macro for tolerant wire enums. |
| crates/cmtraceopen-parser/src/lib.rs | Registers the new internal wire module. |
| crates/cmtraceopen-parser/src/intune/portal/windows/mod.rs | Introduces Windows portal module root. |
| crates/cmtraceopen-parser/src/intune/portal/windows/company_portal/package_state/redaction.rs | Implements opt-in redacted export projection (clone, mask identity/path data). |
| crates/cmtraceopen-parser/src/intune/portal/windows/company_portal/package_state/models.rs | Defines versioned capture schema + canonicalization helper for deterministic JSON blobs. |
| crates/cmtraceopen-parser/src/intune/portal/windows/company_portal/package_state/mod.rs | Package-state module entrypoint: parse, preserve unknown fields, and derive findings. |
| crates/cmtraceopen-parser/src/intune/portal/windows/company_portal/package_state/legacy.rs | Experimental legacy Format-List importer with refusal semantics and locale gating. |
| crates/cmtraceopen-parser/src/intune/portal/windows/company_portal/package_state/findings.rs | Deterministic findings layer with coverage gating and schema/absence rules. |
| crates/cmtraceopen-parser/src/intune/portal/windows/company_portal/mod.rs | Exposes the Windows Company Portal module tree. |
| crates/cmtraceopen-parser/src/intune/portal/mod.rs | Adds intune::portal root module documentation and exports. |
| crates/cmtraceopen-parser/src/intune/mod.rs | Exposes the new portal module from the Intune surface. |
| crates/cmtraceopen-parser/src/esp/models.rs | Switches ESP schema enums to use shared raw_preserving_string_enum!. |
| crates/cmtraceopen-parser/src/collector/profile.rs | Adds a contract test asserting the embedded AppX adapter emits versioned JSON (not Format-List). |
| crates/cmtraceopen-parser/src/collector/profile_data.json | Updates embedded collector command from Format-List text to versioned JSON capture output. |
…odule Before: `raw_preserving_string_enum!` lived as a private `macro_rules!` at the top of `esp/models.rs`. It is the crate's one mechanism for round-tripping unrecognized wire values losslessly, but any other schema module that wanted it had to either make `esp` internals public or hand-copy the macro body. This moves the macro to a new crate-internal `src/wire.rs` and has `esp/models.rs` import it via `pub(crate) use`. The expansion now names serde through absolute paths (`::serde::Serialize`, `::core::result::Result`), so a caller no longer has to arrange for the right traits to be in scope. This is the right seam because tolerance to unknown wire values is a crate-wide property of public capture schemas, not an ESP detail. Keeping exactly one copy means a future fix to the round-trip behavior lands everywhere at once. Behavior is unchanged: the generated code is identical, and the `esp` public API is untouched. Verified with `cargo check --locked --all-targets` (exit 0) and `cargo clippy --locked --all-targets -- -D warnings` (exit 0). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ence
Before: the collector ran `Get-AppxPackage -AllUsers | ... | Format-List` and
wrote the result to `appx-intune-packages.txt`. Nothing parsed that file, and
nothing could have parsed it safely: `Format-List` is a display rendering whose
labels are localized and whose field order, wrapping, and truncation vary by
console width and PowerShell version. There was no notion of query coverage at
all, so an unelevated run that enumerated nothing was indistinguishable from a
device with no Company Portal installed.
This adds `intune::portal::windows::company_portal::package_state`, a versioned
JSON capture schema plus a deterministic findings layer over it, and switches
the collector to emit that schema.
The capture (`schemaVersion 1`) carries provenance (capture time, adapter
version, Windows/PowerShell version, locale), an explicit `commandStatus`, and
per-scope `scopeCoverage`. Package rows carry name/family/full name, version,
architecture, publisher, signature kind, status, install state, and scope. Every
string enum round-trips unrecognized values through `Unknown(String)`, unknown
adapter fields are folded into a per-row `raw` bag, and a `schemaVersion` newer
than this build is not an error: the raw document is preserved, no package facts
are claimed, and an `UnsupportedSchema` finding is emitted.
The load-bearing rule is that absence is a claim about the capture, not the
device. `PackageAbsentFromCapturedScope` is emitted only when `commandStatus` is
`completed` AND the scope's coverage is `complete`. A denied, failed, capped,
timed-out, or never-queried scope produces `IncompleteQuery` instead. Both
directions are tested. `VersionMismatch` compares against an
`ExpectedPackageFact` the caller supplies; the crate never looks a version up.
`legacy.rs` can import old `Format-List` text, but refuses rather than guessing:
a missing or non-English locale, or any line that is not an unambiguous
label/value pair (which is what wrapping and truncation look like), returns
`LegacyImportOutcome::Refused` with a reason and a line number. Refusal is a
distinct variant from an imported-but-empty capture, imports are stamped
`source: legacyFormatList`, their coverage is `partial` so absence stays
structurally unclaimable, and every derived finding is capped to low confidence.
`redaction.rs` is an opt-in projection, never a mutation: install locations,
capture error messages, user identifiers (pseudonymized as `[redacted-user-N]`
from a `BTreeSet` so indices are stable), and user-profile paths inside
preserved raw blobs are masked. Publisher CN values and versions are left alone
so the export stays diagnostically useful.
This is the right seam because the schema, not the PowerShell rendering, is the
public contract. The pure crate does no I/O, no lookups, and no remediation; the
Windows adapter's only job is to fill the envelope truthfully, including
reporting its own failure.
Two things the design notes got wrong about this codebase, both found by tests:
1. `serde_json` IS built with `preserve_order` in the workspace build (`evtx`
enables it and Cargo unifies features), so `serde_json::Map` is an `IndexMap`
there and a `BTreeMap` under `cargo test -p cmtraceopen-parser`. The golden
serialization tests passed per-crate and failed workspace-wide. Preserved
blobs are now canonicalized with sorted keys on the way in, so the bytes are
identical under either build.
2. `LegacyImportOutcome::Imported` has to box its payload;
`clippy::large_enum_variant` rejects the unboxed form under `-D warnings`.
Verified on macOS (commands run from the repo root):
npx tsc --noEmit exit 0
cargo check --locked --all-targets exit 0
cargo clippy --locked --all-targets -- -D warnings exit 0
cargo check --locked -p cmtraceopen-parser \
--target wasm32-unknown-unknown exit 0
cargo test --locked -p cmtraceopen-parser \
--test company_portal_windows_package_state 19 passed, 0 failed
cargo test --locked 1382 passed, 0 failed
(baseline on this branch point: 1362 passed, 0 failed; +19 integration
tests and +1 collector profile test)
The collector command itself was executed end to end via `pwsh` on macOS, where
`Get-AppxPackage` does not exist: it produced a valid envelope with
`commandStatus: "failed"`, scope coverage `failed`, and the error detail, which
is exactly the shape the contract requires of a failed capture.
Not verifiable on this host: the `#[cfg(all(test, target_os = "windows"))]` test
in `src-tauri/src/collector/mod.rs`, which runs the embedded adapter command and
parses its output. It was compile-checked and clippy-checked by temporarily
retargeting the cfg to macOS, but it has never been executed; the Windows CI job
is its first real run.
Closes #367
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
adamgell
force-pushed
the
claude/367-company-portal-package-state
branch
from
July 31, 2026 15:37
c1bd7d9 to
1a3eac5
Compare
Owner
Author
Code reviewFound 1 issue:
🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
The redacted export numbered user pseudonyms by lexicographic position in a
BTreeSet of the raw identifier values, and a second pass re-pseudonymized the
pseudonyms it had just produced. Because "[redacted-user-10]" sorts before
"[redacted-user-1]" ('0' < ']'), any capture holding ten or more distinct
identifiers came back renumbered, so applying the projection to its own output
was not a no-op. That contradicted the function's own doc comment and broke the
stable-pseudonym guarantee across repeated exports of one capture. The existing
test used two identifiers, where lexicographic and numeric order coincide, so it
could not catch this.
An already-redacted value is now terminal: is_redaction_marker recognizes both
the plain mask and a numbered pseudonym, those values are excluded from the
numbering, and every write site leaves them untouched. Idempotence therefore no
longer depends on how many identifiers a capture holds.
Also carries the parseHints convention forward onto appx-info. Every other
JSON-emitting command in the collection profile declares a "json" hint, and the
evidence-bundle dialog searches on it, so converting this artifact from
Format-List text to JSON without the hint left it undiscoverable by that search.
Verified: the new test fails against the previous implementation and passes
against this one. cargo test --locked 1383 passed / 0 failed (1382 before, plus
the regression test); cargo clippy --locked --all-targets -- -D warnings exit 0;
cargo check --locked -p cmtraceopen-parser --target wasm32-unknown-unknown exit
0; npx tsc --noEmit exit 0.
Refs #367
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Owner
Author
|
Fixed in f5a5feb. An already-redacted value is now terminal: Added One correction to my review comment above: I wrote that the bug also contradicted the adjacent "order-independent" claim. That was wrong. The pseudonyms genuinely are independent of the order rows appear in, which is what that comment meant. Only the idempotence claim was broken. Also carried the Gates on f5a5feb: |
push_absence_findings skipped an app entirely as soon as the capture held any row for it, then emitted a per-scope message. So an app registered only in currentUser, with both currentUser and allUsers enumerated completely, produced no finding at all, even though it really is absent from allUsers. A per-user-only Company Portal registration is a genuine deployment signal and the contract was staying silent about it. Absence is now decided per scope: for each completely enumerated scope, the app is absent when no row for it lists that scope. A row carrying no scope attribution suppresses the claim for that app entirely, because an unattributable row might be the very registration in question, and over-claiming absence is the failure mode this contract exists to prevent. The zero-row case is unchanged. The doc comment also claimed absence was limited to apps the caller asked about, while Company Portal has always been checked unconditionally. Reworded to match what the code does. Reported by GitHub Copilot on PR #395. Verified: package_state_absence_is_decided_per_scope_not_per_app fails against the previous implementation and passes against this one. cargo test --locked 1385 passed / 0 failed in the parser crate and workspace; cargo clippy --locked --all-targets -- -D warnings exit 0; cargo check --locked -p cmtraceopen-parser --target wasm32-unknown-unknown exit 0; npx tsc --noEmit exit 0. Note: src-tauri/tests/cmtlog_parser.rs::header_entry_parsed_correctly flakes independently of this change. TempLogFixture names its temp dir from SystemTime::now().as_nanos(), parallel tests collide on one directory, and Drop removes it out from under the other test. Pre-existing and tracked separately. Refs #367 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (5)
crates/cmtraceopen-parser/src/intune/portal/windows/company_portal/package_state/findings.rs (1)
407-408: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCorrect the doc comment: Company Portal is checked unconditionally.
The doc comment states that absence is claimed "only for apps the caller asked about". Line 424 always seeds the list with
PortalApp::CompanyPortal, whether or not the caller supplied a fact. The inline comment on line 422 states the real rule. Align the doc comment with the code.♻️ Proposed fix
-/// Absence is claimed only for apps the caller asked about, and only against a -/// scope the adapter proved it enumerated completely. +/// Absence is claimed for Company Portal, which is the subject of this +/// contract, plus any app the caller supplied a fact for. Every claim is made +/// only against a scope the adapter proved it enumerated completely.🤖 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/package_state/findings.rs` around lines 407 - 408, Update the documentation comment above the relevant findings logic to state that absence is claimed for caller-requested apps and always for Company Portal, limited to scopes the adapter proved it enumerated completely. Keep the behavior and inline comment unchanged.crates/cmtraceopen-parser/src/intune/portal/windows/company_portal/package_state/legacy.rs (1)
360-369: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRemove the
expectfromcamel_case_enum.The
expectmessage is correct only for enums thatraw_preserving_string_enumgenerates. The function signature accepts anyT: Deserialize, so a later call with a strict enum panics inside a pure parser on untrusted text. RequireDefaultand fall back instead of panicking.♻️ Proposed fix
-fn camel_case_enum<T: for<'de> Deserialize<'de>>(value: &str) -> T { +fn camel_case_enum<T: for<'de> Deserialize<'de> + Default>(value: &str) -> T { let mut chars = value.chars(); let camel = match chars.next() { Some(first) => first.to_lowercase().collect::<String>() + chars.as_str(), None => String::new(), }; - serde_json::from_value(Value::String(camel)) - .expect("raw-preserving enums accept any string value") + // Raw-preserving enums accept any string, so this fallback is unreachable + // for them. It keeps a strict enum from panicking in a pure parser. + serde_json::from_value(Value::String(camel)).unwrap_or_default() }
PackageArchitecture,PackageSignatureKind, andPackageStatusneed aDefaultimpl for this change. Alternatively keep the current signature and drop the panic by returningOption<T>.🤖 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/package_state/legacy.rs` around lines 360 - 369, Update camel_case_enum to require T: Default and replace the expect-based deserialization with a non-panicking fallback to T::default() when conversion fails. Add Default implementations for PackageArchitecture, PackageSignatureKind, and PackageStatus, preserving the existing camel-case transformation and successful deserialization behavior.crates/cmtraceopen-parser/src/intune/portal/windows/company_portal/package_state/models.rs (1)
296-314: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test that keeps
KNOWN_PACKAGE_ROW_FIELDSin sync withPackageRow.The list duplicates the
PackageRowfield names by hand. If someone adds a field toPackageRowand forgets this list, the new field parses into its typed field and is also copied intoraw, so the same value appears twice in the capture. A cheap guard is a test that serializesPackageRow::default()and compares the resulting object keys with this list.♻️ Suggested guard test
#[test] fn known_package_row_fields_match_serialized_keys() { let value = serde_json::to_value(PackageRow::default()).expect("row serializes"); let mut keys: Vec<&str> = value .as_object() .expect("row is an object") .keys() .map(String::as_str) .collect(); keys.sort_unstable(); let mut known = KNOWN_PACKAGE_ROW_FIELDS.to_vec(); known.sort_unstable(); assert_eq!(keys, known); }Note: this test only works when no field is skipped during serialization.
🤖 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/package_state/models.rs` around lines 296 - 314, Add a unit test near PackageRow and KNOWN_PACKAGE_ROW_FIELDS that serializes PackageRow::default(), extracts and sorts its object keys, sorts a copy of KNOWN_PACKAGE_ROW_FIELDS, and asserts the lists match. Ensure the test uses the existing serde serialization behavior and remains valid only when all PackageRow fields are serialized.crates/cmtraceopen-parser/src/intune/portal/windows/company_portal/package_state/redaction.rs (1)
53-79: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSensitivity classification is recorded but never consulted.
mask()(Line 75-79) blanks any populatedPackageStateClassifiedStringregardless of itssensitivityvalue, and theuser_identifiersubstitution (Line 55-62) redacts any non-marker value regardless of itssensitivitytoo. Right now this is safe, because the only values wired up aresensitive(installLocation) orrestricted(userIdentifier). But the field exists specifically to classify values, and this logic ignores it.If a future producer attaches a lower-sensitivity (for example, public) classified string to one of these fields to preserve a non-identity value, this code still discards it, contradicting the module's own stated goal to avoid over-redaction of diagnostic value. Gate the masking decision on
sensitivityinstead of on presence alone.♻️ Proposed direction for sensitivity-aware masking
-fn mask(value: &mut Option<PackageStateClassifiedString>) { - if let Some(classified) = value.as_mut() { - classified.value = REDACTED.to_string(); - } -} +fn mask(value: &mut Option<PackageStateClassifiedString>) { + if let Some(classified) = value.as_mut() { + if classified.sensitivity.requires_redaction() { + classified.value = REDACTED.to_string(); + } + } +}(Requires
models.rsto expose an equivalent predicate on thesensitivitytype.)🤖 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/package_state/redaction.rs` around lines 53 - 79, Update mask and the user_identifier redaction in the package redaction flow to consult each classified string’s sensitivity instead of redacting solely based on field presence or marker status. Add or reuse a predicate exposed by the sensitivity type in models.rs, and preserve values whose sensitivity does not require masking while continuing to redact sensitive or restricted values and existing redaction markers.crates/cmtraceopen-parser/tests/company_portal_windows_package_state.rs (1)
514-517: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign the assertion message with the implemented ordering.
sort_findingsorders byPackageStateFindingKind::rank()and finding ID, not by severity. Change the message to"findings must be emitted in stable kind order". Do not sort severities ascending;PackageStateFindingSeverityordersInfo < Warning < Error.🤖 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_package_state.rs` around lines 514 - 517, Update the assertion message in the ranks ordering check to “findings must be emitted in stable kind order,” matching sort_findings’ rank-and-ID ordering. Keep the existing collection and sorting logic unchanged; do not introduce severity-based sorting.
🤖 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/collector/profile_data.json`:
- Around line 1302-1307: Update the access-denial classification in the
PowerShell catch block for the AppX capture command: retain the
UnauthorizedAccessException check, then also inspect FullyQualifiedErrorId or
Exception.HResult for E_ACCESSDENIED/0x80070005 before falling back to message
matching. Ensure localized access-denied failures set commandStatus to
accessDenied and coverage to denied rather than failed.
In `@crates/cmtraceopen-parser/src/collector/profile.rs`:
- Around line 173-186: Update the required command fragments in the test around
the appx-info adapter command to assert the configured JSON depth as the
combined flag and value “-Depth 6”, replacing the current check that only
verifies “-Depth”. Keep the existing assertions for the other command options
unchanged.
In
`@crates/cmtraceopen-parser/src/intune/portal/windows/company_portal/package_state/findings.rs`:
- Around line 231-289: The derived finding text bypasses the redaction
projection and can expose sensitive values. In
crates/cmtraceopen-parser/src/intune/portal/windows/company_portal/package_state/findings.rs:231-289,
update the finding construction to avoid interpolating
PackageCaptureError.message and PackageScopeCoverage.detail, or derive findings
from the redacted capture before export. In
crates/cmtraceopen-parser/src/intune/portal/windows/company_portal/package_state/legacy.rs:137-146,
replace the raw source-line content in LegacyRefusal.detail with only the line
number and character count.
- Around line 386-390: Update the version-mismatch finding ID construction in
the findings push logic to include fact.source alongside the row index and
expected version. Preserve the existing ID prefix and ensure different sources
produce distinct IDs.
- Around line 312-327: Update the status check in the package-state findings
logic so PackageStatus::Unknown with an empty value is treated as unreported and
does not create a PackageStatusProblem finding. Continue reporting non-Ok
statuses when they contain an actual status value, while preserving the existing
finding construction for those cases.
In
`@crates/cmtraceopen-parser/src/intune/portal/windows/company_portal/package_state/legacy.rs`:
- Around line 261-276: Update LegacyRecord::to_row to detect when the
identifying “Name” label appears more than once, case-insensitively, before
constructing the PackageRow. Return LegacyRefusalReason::AmbiguousRecord with
the record’s line context instead of emitting a row; keep the existing
missing/blank Name handling unchanged.
In `@src-tauri/src/collector/mod.rs`:
- Around line 43-48: Update the adapter execution assertion in the collector
test to parse and validate stdout even when the process exits non-zero, rather
than requiring output.status.success(). Preserve rejection of invalid execution
output, and include output.status.code() as exit_code in the parse-failure
message alongside the existing stderr details.
- Around line 50-52: Update the adapter stdout decoding before
parse_package_state_capture: decode bytes as UTF-8 with an encoding_rs
Windows-1252 fallback instead of String::from_utf8_lossy, preserving publisher
strings and paths without replacement-character corruption.
---
Nitpick comments:
In
`@crates/cmtraceopen-parser/src/intune/portal/windows/company_portal/package_state/findings.rs`:
- Around line 407-408: Update the documentation comment above the relevant
findings logic to state that absence is claimed for caller-requested apps and
always for Company Portal, limited to scopes the adapter proved it enumerated
completely. Keep the behavior and inline comment unchanged.
In
`@crates/cmtraceopen-parser/src/intune/portal/windows/company_portal/package_state/legacy.rs`:
- Around line 360-369: Update camel_case_enum to require T: Default and replace
the expect-based deserialization with a non-panicking fallback to T::default()
when conversion fails. Add Default implementations for PackageArchitecture,
PackageSignatureKind, and PackageStatus, preserving the existing camel-case
transformation and successful deserialization behavior.
In
`@crates/cmtraceopen-parser/src/intune/portal/windows/company_portal/package_state/models.rs`:
- Around line 296-314: Add a unit test near PackageRow and
KNOWN_PACKAGE_ROW_FIELDS that serializes PackageRow::default(), extracts and
sorts its object keys, sorts a copy of KNOWN_PACKAGE_ROW_FIELDS, and asserts the
lists match. Ensure the test uses the existing serde serialization behavior and
remains valid only when all PackageRow fields are serialized.
In
`@crates/cmtraceopen-parser/src/intune/portal/windows/company_portal/package_state/redaction.rs`:
- Around line 53-79: Update mask and the user_identifier redaction in the
package redaction flow to consult each classified string’s sensitivity instead
of redacting solely based on field presence or marker status. Add or reuse a
predicate exposed by the sensitivity type in models.rs, and preserve values
whose sensitivity does not require masking while continuing to redact sensitive
or restricted values and existing redaction markers.
In `@crates/cmtraceopen-parser/tests/company_portal_windows_package_state.rs`:
- Around line 514-517: Update the assertion message in the ranks ordering check
to “findings must be emitted in stable kind order,” matching sort_findings’
rank-and-ID ordering. Keep the existing collection and sorting logic unchanged;
do not introduce severity-based sorting.
🪄 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: 4bcfc608-3d89-45b4-bbed-9e8dc4929e1b
📒 Files selected for processing (33)
crates/cmtraceopen-parser/src/collector/profile.rscrates/cmtraceopen-parser/src/collector/profile_data.jsoncrates/cmtraceopen-parser/src/esp/models.rscrates/cmtraceopen-parser/src/intune/mod.rscrates/cmtraceopen-parser/src/intune/portal/mod.rscrates/cmtraceopen-parser/src/intune/portal/windows/company_portal/mod.rscrates/cmtraceopen-parser/src/intune/portal/windows/company_portal/package_state/findings.rscrates/cmtraceopen-parser/src/intune/portal/windows/company_portal/package_state/legacy.rscrates/cmtraceopen-parser/src/intune/portal/windows/company_portal/package_state/mod.rscrates/cmtraceopen-parser/src/intune/portal/windows/company_portal/package_state/models.rscrates/cmtraceopen-parser/src/intune/portal/windows/company_portal/package_state/redaction.rscrates/cmtraceopen-parser/src/intune/portal/windows/mod.rscrates/cmtraceopen-parser/src/lib.rscrates/cmtraceopen-parser/src/wire.rscrates/cmtraceopen-parser/tests/company_portal_windows_package_state.rscrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/package_state/absent-after-complete-all-users-capture/capture.jsoncrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/package_state/access-denied-incomplete-query/capture.jsoncrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/package_state/command-failure/capture.jsoncrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/package_state/deterministic-serialization/capture.jsoncrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/package_state/deterministic-serialization/golden.jsoncrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/package_state/installed-company-portal-and-authenticator/capture.jsoncrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/package_state/installed-company-portal/capture.jsoncrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/package_state/legacy-format-list-english/packages.txtcrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/package_state/legacy-format-list-refused/non-english.txtcrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/package_state/legacy-format-list-refused/wrapped.txtcrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/package_state/malformed-json/capture.jsoncrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/package_state/multiple-registrations/capture.jsoncrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/package_state/package-status-problem/capture.jsoncrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/package_state/per-user-only-registration/capture.jsoncrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/package_state/privacy-paths/capture.jsoncrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/package_state/privacy-paths/golden-redacted.jsoncrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/package_state/unknown-future-schema/capture.jsonsrc-tauri/src/collector/mod.rs
Six issues from the CodeRabbit review of PR #395. Each was verified against the code before being accepted, and each fix carries a test that fails against the previous implementation. Privacy. Finding messages interpolated the adapter's error message and scope-coverage detail, both classified sensitive in the capture schema. PackageStateFinding is a separate type that redacted_package_state_export never sees, so a redacted export still carried whatever those fields held: the access-denied fixture's message contains a real-looking profile path. Findings now carry the stable error code only, and the free text stays in the capture where redaction covers it. LegacyRefusal.detail quoted the offending source line for the same reason and now reports its position and length instead. Correctness. PackageRow uses #[serde(default)], so a row omitting `status` deserialized to an empty Unknown and was reported as an Error-severity package health problem: a missing field became an invented fault. An unreported status is now silent. Version-mismatch finding ids omitted the fact source, so two facts naming the same app and expected version from different sources collided on one id with two different messages; the source is now part of the id. Legacy import. A missing blank separator merged two Format-List records into one, and the import reported success carrying the first record's Name with both records' fields mixed. This module refuses wrapped and truncated input precisely so a bad read never looks like a good one, so a repeated identifying label now refuses with AmbiguousRecord. Collector. Access denial was classified by matching English words in the error message, so a localized Windows host reported accessDenied as failed. The catch block now checks FullyQualifiedErrorId and the E_ACCESSDENIED HResult before falling back to message text. The profile test pinned only the -Depth flag and now pins -Depth 6, since a lower value silently truncates scopeCoverage and installLocation. Windows adapter test. It asserted a successful exit code before parsing, which contradicted its own acceptance of Failed and AccessDenied: those can only be observed when the script exits 0 and reports the outcome inside the JSON. It now parses first and treats a non-zero exit with a parseable capture as valid. Stdout was decoded with from_utf8_lossy, which turns Windows-1252 bytes in a publisher string or install path into U+FFFD; it now uses detect_encoding plus decode_bytes, the UTF-8 with Windows-1252 fallback CLAUDE.md requires. Verified: all five new parser tests fail against the previous implementation and pass against this one. cargo test --locked 1390 passed / 0 failed (1385 before); cargo clippy --locked --all-targets -- -D warnings exit 0; cargo check --locked -p cmtraceopen-parser --target wasm32-unknown-unknown exit 0; npx tsc --noEmit exit 0. The edited Windows-gated test still cannot run on this macOS host. It was type-checked by temporarily retargeting its cfg to macos and running cargo check and clippy with --all-targets, then restoring the Windows gate. Windows CI remains its first real execution. Refs #367 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/cmtraceopen-parser/tests/company_portal_windows_package_state.rs (1)
897-914: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that the fixtures actually supply sensitive text.
Both privacy checks are conditional. If
capture.capture.errorbecomesNone, or everycoverage.detailbecomesNone, the loop body asserts nothing and the test still passes. The literal"jrivera"andC:\Userscheck has the same weakness, because it also passes when the fixture no longer contains those values.Add a precondition so the test fails when the fixture stops carrying sensitive text.
♻️ Proposed precondition assertion
for source in [ACCESS_DENIED, COMMAND_FAILURE] { let capture = parse(source); let findings = derive_package_state_findings(&capture, &[]); + let sensitive: Vec<&str> = capture + .capture + .error + .as_ref() + .map(|error| error.message.as_str()) + .into_iter() + .chain( + capture + .capture + .scope_coverage + .iter() + .filter_map(|coverage| coverage.detail.as_deref()), + ) + .collect(); + assert!( + !sensitive.is_empty(), + "fixture must supply adapter free text for this test to mean anything" + ); let messages = findings🤖 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_package_state.rs` around lines 897 - 914, Add precondition assertions in the privacy test around the existing checks to verify the fixture contains each expected sensitive value: the adapter error message, at least one scope-coverage detail, and the identity/profile-path text represented by “jrivera” and “C:\Users”. Keep the existing non-leak assertions unchanged so the test fails both when sensitive fixture data disappears and when it is exposed in finding messages.
🤖 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.
Nitpick comments:
In `@crates/cmtraceopen-parser/tests/company_portal_windows_package_state.rs`:
- Around line 897-914: Add precondition assertions in the privacy test around
the existing checks to verify the fixture contains each expected sensitive
value: the adapter error message, at least one scope-coverage detail, and the
identity/profile-path text represented by “jrivera” and “C:\Users”. Keep the
existing non-leak assertions unchanged so the test fails both when sensitive
fixture data disappears and when it is exposed in finding messages.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2aefa967-7206-4f9b-bc04-f8fb8ea51bb3
📒 Files selected for processing (6)
crates/cmtraceopen-parser/src/collector/profile.rscrates/cmtraceopen-parser/src/collector/profile_data.jsoncrates/cmtraceopen-parser/src/intune/portal/windows/company_portal/package_state/findings.rscrates/cmtraceopen-parser/src/intune/portal/windows/company_portal/package_state/legacy.rscrates/cmtraceopen-parser/tests/company_portal_windows_package_state.rssrc-tauri/src/collector/mod.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/cmtraceopen-parser/src/collector/profile.rs
- src-tauri/src/collector/mod.rs
- crates/cmtraceopen-parser/src/intune/portal/windows/company_portal/package_state/findings.rs
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 33 out of 33 changed files in this pull request and generated no new comments.
Suppressed comments (2)
crates/cmtraceopen-parser/src/intune/portal/windows/company_portal/package_state/mod.rs:71
parse_package_state_capturecurrently deserializesPackageStateCapturewith#[serde(default)], so documents that omit required fields likecaptureorpackageswill silently default them (e.g.,packages: []). If such a partially-shaped document also reportscommandStatus: completedandscopeCoverage: complete,derive_package_state_findingscan incorrectly claim package absence based on missing data. Consider validating thatcaptureis an object andpackagesis an array before deserializing, and treat missing fields asInvalidBody.
let mut capture: PackageStateCapture = serde_json::from_value(document.clone()).map_err(
|error| PackageStateError::InvalidBody {
version: schema_version,
detail: error.to_string(),
},
)?;
capture.schema_version = schema_version;
capture.raw_document = None;
preserve_unknown_package_fields(&document, &mut capture);
Ok(capture)
crates/cmtraceopen-parser/src/collector/profile_data.json:1304
- In the embedded PowerShell adapter command, several values are forced through
[string]...(e.g.publisher = [string]$p.Publisher,code = [string]$_.FullyQualifiedErrorId). In PowerShell,[string]$nullbecomes an empty string, so optional fields will serialize as""instead ofnull/absent. Since the capture schema models these as optional (Option<String>), emitting""can blur the distinction between “unknown/unreported” and “present but empty”, and it can create misleading downstream facts.
"arguments": ["-NoProfile", "-Command", "$ErrorActionPreference = 'Stop'; $cc = { param($v) $s = [string]$v; if ($s.Length -gt 0) { $s.Substring(0,1).ToLowerInvariant() + $s.Substring(1) } else { '' } }; $rows = @(); $commandStatus = 'completed'; $coverage = 'complete'; $captureError = $null; try { $rows = @(Get-AppxPackage -AllUsers -ErrorAction Stop | Where-Object { $_.Name -match 'CompanyPortal|IntuneCompanyPortal|Authenticator' }) } catch { $rows = @(); $msg = [string]$_.Exception.Message; if ($_.Exception -is [System.UnauthorizedAccessException] -or ([string]$_.FullyQualifiedErrorId) -match 'UnauthorizedAccess|AccessDenied' -or (($_.Exception.HResult -band 0xFFFFFFFF) -eq 0x80070005) -or $msg -match 'denied|elevated|administrator') { $commandStatus = 'accessDenied'; $coverage = 'denied' } else { $commandStatus = 'failed'; $coverage = 'failed' }; $captureError = [ordered]@{ code = [string]$_.FullyQualifiedErrorId; message = $msg } }; $packages = @(); foreach ($p in $rows) { $states = @($p.PackageUserInformation | Where-Object { $_ -ne $null }); $installState = 'notInstalled'; if ($states | Where-Object { $_.InstallState -eq 'Installed' }) { $installState = 'installed' } elseif ($states | Where-Object { $_.InstallState -eq 'Staged' }) { $installState = 'staged' }; $app = 'other'; if ($p.Name -match 'CompanyPortal') { $app = 'companyPortal' } elseif ($p.Name -match 'Authenticator') { $app = 'authenticator' }; $location = $null; if ($p.InstallLocation) { $location = [ordered]@{ value = [string]$p.InstallLocation; sensitivity = 'sensitive' } }; $packages += [ordered]@{ name = [string]$p.Name; familyName = [string]$p.PackageFamilyName; fullName = [string]$p.PackageFullName; version = [string]$p.Version; architecture = (& $cc $p.Architecture); publisher = [string]$p.Publisher; signatureKind = (& $cc $p.SignatureKind); status = (& $cc $p.Status); installState = $installState; scopes = @('allUsers'); userRegistrationCount = $states.Count; installLocation = $location; app = $app } }; $doc = [ordered]@{ schemaVersion = 1; capture = [ordered]@{ capturedAtUtc = (Get-Date).ToUniversalTime().ToString('o'); adapterVersion = 'cmtraceopen-collector-appx/1'; commandStatus = $commandStatus; windowsBuild = [string][System.Environment]::OSVersion.Version; powerShellVersion = [string]$PSVersionTable.PSVersion; locale = [string](Get-Culture).Name; source = 'json'; scopeCoverage = @([ordered]@{ scope = 'allUsers'; status = $coverage; detail = $null }); error = $captureError }; packages = @($packages) }; $doc | ConvertTo-Json -Depth 6 -Compress"],
…king Four CodeRabbit nitpicks on the package-state contract, each verified before being accepted. One stale nitpick about the push_absence_findings doc comment was already fixed in 92c1704 and is skipped. camel_case_enum called expect on the deserialize result. The raw-preserving enums it is used with accept any string, so it cannot fail today, but the bound was only Deserialize: a later call with a strict enum would panic inside a pure parser on untrusted text. It now requires Default and falls back. Default is implemented for the four package enums as an empty Unknown, which is what PackageRow::default already constructed by hand. The redaction projection masked and pseudonymized on presence alone, ignoring the sensitivity field that exists precisely to classify these values. Every value wired up today is sensitive or restricted, so behavior is unchanged, but a producer marking a value public would have had it discarded anyway. That is the over-redaction this module documents itself as avoiding. Masking is now gated on the classification. Two test-quality fixes. The privacy test's assertions were all conditional, so a fixture that stopped carrying sensitive text would leave the test passing while proving nothing; it now asserts the fixture still carries identity and a profile path before asserting no finding repeats them. The ordering assertion claimed findings are emitted most-severe first, but sort_findings orders by kind rank and id; the message now says what the code does. Added an inline models test comparing the serialized PackageRow shape against KNOWN_PACKAGE_ROW_FIELDS. That list is maintained by hand and decides what gets folded into `raw`, so a field added to the struct but forgotten there would appear twice in one capture. Verified: cargo test --locked 1391 passed / 0 failed (1390 before); cargo clippy --locked --all-targets -- -D warnings exit 0; cargo check --locked -p cmtraceopen-parser --target wasm32-unknown-unknown exit 0; npx tsc --noEmit exit 0. Refs #367 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 33 out of 33 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src-tauri/src/collector/mod.rs:43
- This test runs the PowerShell adapter via
Command::output()without applying the command item's configuredtimeout_secs. If theGet-AppxPackagecall hangs (e.g., PowerShell host issues, AppX subsystem stalls), the Windows CI job can hang indefinitely instead of failing fast.
Consider spawning the process with stdout/stderr piped, polling with try_wait() until item.timeout_secs elapses, and killing the child on timeout (then failing the test with diagnostics).
crates/cmtraceopen-parser/src/collector/profile_data.json:1304
- The
$cchelper only lowercases the first character to convert PowerShell’s PascalCase tokens into the schema’s camelCase. That breaks all-caps architecture values: e.g.,ARM64becomesaRM64, which will deserialize asPackageArchitecture::Unknown("aRM64")instead of the intendedarm64variant.
To keep ARM/ARM64 devices on the typed variants, tweak $cc to fully-lowercase strings that are entirely [A-Z0-9]+ (or otherwise special-case ARM/ARM64) while preserving the current behavior for NeedsRemediation-style tokens.
"arguments": ["-NoProfile", "-Command", "$ErrorActionPreference = 'Stop'; $cc = { param($v) $s = [string]$v; if ($s.Length -gt 0) { $s.Substring(0,1).ToLowerInvariant() + $s.Substring(1) } else { '' } }; $rows = @(); $commandStatus = 'completed'; $coverage = 'complete'; $captureError = $null; try { $rows = @(Get-AppxPackage -AllUsers -ErrorAction Stop | Where-Object { $_.Name -match 'CompanyPortal|IntuneCompanyPortal|Authenticator' }) } catch { $rows = @(); $msg = [string]$_.Exception.Message; if ($_.Exception -is [System.UnauthorizedAccessException] -or ([string]$_.FullyQualifiedErrorId) -match 'UnauthorizedAccess|AccessDenied' -or (($_.Exception.HResult -band 0xFFFFFFFF) -eq 0x80070005) -or $msg -match 'denied|elevated|administrator') { $commandStatus = 'accessDenied'; $coverage = 'denied' } else { $commandStatus = 'failed'; $coverage = 'failed' }; $captureError = [ordered]@{ code = [string]$_.FullyQualifiedErrorId; message = $msg } }; $packages = @(); foreach ($p in $rows) { $states = @($p.PackageUserInformation | Where-Object { $_ -ne $null }); $installState = 'notInstalled'; if ($states | Where-Object { $_.InstallState -eq 'Installed' }) { $installState = 'installed' } elseif ($states | Where-Object { $_.InstallState -eq 'Staged' }) { $installState = 'staged' }; $app = 'other'; if ($p.Name -match 'CompanyPortal') { $app = 'companyPortal' } elseif ($p.Name -match 'Authenticator') { $app = 'authenticator' }; $location = $null; if ($p.InstallLocation) { $location = [ordered]@{ value = [string]$p.InstallLocation; sensitivity = 'sensitive' } }; $packages += [ordered]@{ name = [string]$p.Name; familyName = [string]$p.PackageFamilyName; fullName = [string]$p.PackageFullName; version = [string]$p.Version; architecture = (& $cc $p.Architecture); publisher = [string]$p.Publisher; signatureKind = (& $cc $p.SignatureKind); status = (& $cc $p.Status); installState = $installState; scopes = @('allUsers'); userRegistrationCount = $states.Count; installLocation = $location; app = $app } }; $doc = [ordered]@{ schemaVersion = 1; capture = [ordered]@{ capturedAtUtc = (Get-Date).ToUniversalTime().ToString('o'); adapterVersion = 'cmtraceopen-collector-appx/1'; commandStatus = $commandStatus; windowsBuild = [string][System.Environment]::OSVersion.Version; powerShellVersion = [string]$PSVersionTable.PSVersion; locale = [string](Get-Culture).Name; source = 'json'; scopeCoverage = @([ordered]@{ scope = 'allUsers'; status = $coverage; detail = $null }); error = $captureError }; packages = @($packages) }; $doc | ConvertTo-Json -Depth 6 -Compress"],
Owner
Author
Review cycle convergedFinal head Cycle: The two that mattered most:
Copilot's final review at The Windows-gated adapter test has now actually executed. From the Flagging one thing for whoever touches this area next: Local gates on Two unrelated defects were found while working this PR and are deliberately not fixed here, both described in the PR body: the |
Owner
Author
|
@coderabbitai review Requesting a re-review anchored to |
|
✅ Action performedReview finished.
|
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.
Closes #367.
Before
crates/cmtraceopen-parser/src/collector/profile_data.jsoncollected Company Portal andAuthenticator package information by running
Get-AppxPackage -AllUsers | ... | Format-Listand writing human-formatted text toappx-intune-packages.txt. Nothing in the repo parsed that file.Format-Listoutput variesby locale, PowerShell version, field ordering, wrapping, and missing values, so it is not a
stable basis for a public parser contract.
What changes
A new
cmtraceopen_parser::intune::portal::windows::company_portal::package_statemodule, thefirst occupant of the
intune::portaltree:COMPANY_PORTAL_PACKAGE_STATE_SCHEMA_VERSION = 1) as thecanonical input, with capture metadata (command status, per-scope coverage, adapter/Windows/
PowerShell versions, locale) and explicit package rows.
package status problem, version mismatch against a separately supplied expected fact,
multiple registrations, incomplete or denied query, malformed capture, and unsupported schema.
Format-Listadapter that requires locale and adapter metadata andrefuses rather than guessing on non-English, wrapped, or merged input. A refusal is a
distinct outcome from an empty capture.
capture error messages, and any user identifier that reaches the model.
ConvertTo-Jsoninstead ofFormat-List, wrapped in try/catch so a denied-AllUsersquery reportscommandStatus: "accessDenied"withdeniedscope coverage rather than a silent empty success.The single most important rule from the issue is enforced and tested in both directions:
absence is claimable only when the command completed and the relevant scope's coverage is
complete. A missing row under a failed, denied, capped, or not-queried scope producescoverage, not a package-absence finding. Absence is also decided per scope rather than per app,
so a registration present only in
currentUseris correctly reported as absent from acompletely enumerated
allUsers.Why this seam
The issue's core complaint is that a public contract should not depend on display-formatted
PowerShell. Splitting it as (native adapter emits versioned JSON) plus (pure crate validates,
reduces, and redacts) keeps the parser crate wasm-compatible and I/O-free while moving the
unstable rendering behind a schema the collector owns. Legacy text import stays reachable for
already-collected bundles but is marked
source: legacyFormatListand never becomes canonical.Two implementation details worth a reviewer's attention:
raw_preserving_string_enum!was promoted fromesp/models.rsto a sharedsrc/wire.rs(3 files touched) so there is exactly one copy rather than a duplicate in the portal tree.
evtxenablesserde_json/preserve_order, and Cargo unifies features workspace-wide. Thatmeans
serde_json::Mapis anIndexMapundercargo test --lockedbut aBTreeMapundercargo test -p cmtraceopen-parser. Preserved raw blobs are canonicalized with sorted keys oningest so golden bytes are identical under either build. Without this, the golden tests pass
per-crate and fail workspace-wide.
Fixture matrix
All 14 scenarios required by the issue are present under
crates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/package_state/<scenario>/,embedded with
include_str!(the crate does no filesystem I/O). A test asserts the scenariodirectory listing matches the expected 14 names, so a dropped scenario fails the build.
Review cycle
This PR was driven through
/code-review, CodeRabbit, and GitHub Copilot. Ten findings wereaccepted and fixed; each fix carries a test confirmed to fail against the previous
implementation. The substantive ones:
[redacted-user-10]sorts before[redacted-user-1])PackageStateFindingcurrentUser-only registration never reported absence from a completely enumeratedallUsersstatusfield deserialized to an emptyUnknownand was reported as an Error-severity package health problemFormat-Listrecords into one row and reported successNamelabel refuses withAmbiguousRecordaccessDeniedasfailedFullyQualifiedErrorIdand theE_ACCESSDENIEDHResult firstfrom_utf8_lossy, corrupting Windows-1252 bytesdetect_encoding+decode_bytes, per CLAUDE.mdLegacyRefusal.detailquoted the offending source line verbatimappx-infoomitted theparseHintsevery other JSON-emitting command declaresVerified
Run locally on
2b9d3d5a, the pushed head, against a clean tree:cargo test --locked -p cmtraceopen-parser --test company_portal_windows_package_statecargo test --lockedcargo clippy --locked --all-targets -- -D warningscargo check --locked -p cmtraceopen-parser --target wasm32-unknown-unknownnpx tsc --noEmitBaseline on
origin/mainfor comparison: 1362 passed, 0 failed, with clippy, wasm, and tsc allexit 0. The branch adds 29 tests.
The Windows-gated test
src-tauri/src/collector/mod.rscarries a test under#[cfg(all(test, target_os = "windows"))]that runs the embedded adapter command and parses itsoutput. It cannot run on the macOS development host, and Windows cross-compilation is blocked
there, so locally it was only type-checked.
It has now run for real on CI and passed. From the
ESP Diagnostics (Windows)job log on2b9d3d5a, step "Test full Windows application workspace":Worth recording for future changes to this area:
Rust MSRV (1.88 / windows-latest)does notexercise it. That job runs
cargo +1.88 check --workspace --all-featureswith no--all-targets, so it never compilescfg(test)code at all. The job that runs this test isESP Diagnostics (Windows), whose final step iscargo test --locked -p cmtrace-open --all-features-cmtrace-openbeing the crate the testlives in.
cargo fmt --check --allwas deliberately not run repo-wide. It is not a CI gate in this repoand the tree has pre-existing drift.
Unrelated issues found while working this PR
Neither is caused by this change and neither is fixed here:
.github/workflows/label-new-items.yml(added by ci: auto-label new issues and PRs by title pattern #383) fails on every new PR withResource not accessible by integration. It declares onlyissues: write; labeling a pullrequest also needs
pull-requests: write.src-tauri/tests/cmtlog_parser.rs::header_entry_parsed_correctlyis flaky, failing roughly1 full-suite run in 5.
TempLogFixture::newnames its temp directory fromSystemTime::now().as_nanos(), parallel tests in that binary collide on one directory, andDropcallsremove_dir_allout from under the other test.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests