refactor(intune): thin Framework v1 extraction (mapping, test support, invariant docs) - #543
Conversation
…, invariant docs) Three items, all behavior-preserving. 1. The IntuneAccessState <-> IntuneArtifactStatus bijection existed as three copies. Both directions now live in intune/evidence.rs beside the enums, with exhaustive matches (no `_` arm) so a new variant is a compile error. Win32 and Microsoft Store call the shared pair directly; Autopilot's declared_status is now the composition of its own lane-local capture-state mapping with the shared one. Verified arm by arm as identical first. 2. Mechanical test-support consolidation into tests/support/mod.rs: scenario_root, artifact_status_for_capture_state (plus an access_state_for_capture_state adapter for Win32), wire, and sorted_evidence_ids. Deliberately left local: Microsoft Store's insertion-ordered evidence_ids (that leaf asserts citation order), and Autopilot's capture_state (returns a lane-local enum via serde, not IntuneArtifactStatus). 3. Docs: an ADR-001 addendum recording that the directional doctrine is shared prose while its mechanism is per-lane, and a new docs/architecture/shared-vs-workload-invariants.md protecting the tested divergences from a future "consistency" PR. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
github-actions
Bot
added
enhancement
📝 WalkthroughWalkthroughThe PR centralizes bidirectional Intune status mappings, updates runtime consumers, adds shared fixture helpers, migrates workload tests, and documents shared invariants and workload-specific evidence rules. ChangesIntune status centralization
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@docs/architecture/decisions/ADR-001-evidence-strength-confidence.md`:
- Around line 24-27: Update the Autopilot row in the architecture decision table
to explicitly define assessability as access_state == Available && parse_state
== Parsed, and express the non-assessable condition as its negation where
applicable. Preserve the existing failure-signal gating and Configuration row
wording.
- Around line 31-42: Normalize all source references in the architecture
documents to verified repository-relative paths. In
docs/architecture/decisions/ADR-001-evidence-strength-confidence.md:31-42,
replace the abbreviated configuration reducer reference with its full path; in
docs/architecture/shared-vs-workload-invariants.md:26-30, :40-47, :63-64,
:81-82, :90-92, :106-108, :138-142, and :146-153, replace every abbreviated
evidence, test-support, reducer, source, Autopilot, Compliance, Store, Win32,
redaction, and Microsoft Store reference with its corresponding full repository
path. Preserve the existing symbols and line-range references while ensuring
every path resolves from the repository root.
In `@docs/architecture/shared-vs-workload-invariants.md`:
- Around line 6-8: Update the lane-count wording in the sentence beginning “Four
lanes” to “Five lanes,” matching the five listed lanes: Win32, Microsoft Store,
Autopilot, Configuration, and Compliance.
- Around line 25-26: Revise the “Additive-only” guidance for the evidence
envelope types in the shared-vs-workload invariants table so it applies only to
wire-format evolution, not general Rust API compatibility. Document the separate
Rust API release policy for these constructible public types, including the
impact of new enum variants and public struct fields, or adjust the types to
hide construction and matching for extensibility.
🪄 Autofix
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: 97eaf6cb-fdeb-49f7-8b48-084e7189c3ca
📒 Files selected for processing (12)
crates/cmtraceopen-parser/src/intune/apps/windows/microsoft_store/reducer.rscrates/cmtraceopen-parser/src/intune/apps/windows/win32/sources.rscrates/cmtraceopen-parser/src/intune/enrollment/windows/autopilot/sources.rscrates/cmtraceopen-parser/src/intune/evidence.rscrates/cmtraceopen-parser/tests/intune_windows_autopilot.rscrates/cmtraceopen-parser/tests/intune_windows_compliance.rscrates/cmtraceopen-parser/tests/intune_windows_configuration.rscrates/cmtraceopen-parser/tests/intune_windows_microsoft_store.rscrates/cmtraceopen-parser/tests/intune_windows_win32.rscrates/cmtraceopen-parser/tests/support/mod.rsdocs/architecture/decisions/ADR-001-evidence-strength-confidence.mddocs/architecture/shared-vs-workload-invariants.md
|
@coderabbitai review |
|
Hermes charter reviewVerdictBehavior-preservation verdict: PASS for the authorized code/test extraction. At exact head Findings (ranked)P1 — The protective architecture document is not accurate/reproducible as written
The requested divergence descriptions themselves are directionally correct after checking the exact-head code: Win32 capped-still-proves with confidence demotion; Autopilot ungated P2 — Full repository formatting is not a clean exact-head gate
Behavior-preservation evidence
Ruling on the
|
…able The invariant document and the ADR-001 addendum were meant to protect deliberate cross-lane divergences from a future consistency PR, but three defects made them unusable as guardrails: - the lane count said "Four" while listing five lanes; - code references were abbreviated to `.../autopilot/reducer.rs`, which a reader cannot resolve from the repository root; - the Autopilot section claimed every reduction path passes through `is_assessable`, then identified `time_basis` as an exception two lines later, and section 3 documents a second one. Paths are now repository-root relative with line numbers, and the assessability claim is replaced by what the reducer actually does: gated status paths, two deliberately inverted coverage-gap filters (reducer.rs:672, :705), and two deliberately ungated paths (time_basis at :541, Detecting at :1350). The ADR's Autopilot trigger states the boolean predicate instead of the ambiguous "envelope not Available + Parsed". An inaccurate guardrail is worse than no guardrail: it invites the reader to distrust the divergences it exists to protect. Documentation only; no code or test changes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ust API The evidence envelope row read as a compatibility guarantee, but none of those types are #[non_exhaustive] and cmtraceopen-parser is published, so appending a variant breaks downstream exhaustive matches even when it is harmless for serialized output. The next table row already depended on that breakage (a new variant becoming a compile error), so the two rows contradicted each other. Scope the guarantee to the wire format and state the crate-release consequence. Also finishes normalizing the table's code references to repository-root paths. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Every code reference in both documents is now repository-root relative and resolves as written. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Pushed doc-accuracy fixes addressing the Hermes P1 and the four CodeRabbit threads. Documentation only, no code or test changes. Lane count — Unresolvable references — every The Additive-only (the Major finding) — agreed, and the document contained its own counter-evidence: the row directly below celebrates that a new variant is a compile error, which is exactly the downstream breakage the row above implied was safe. None of those types are Note for the record: I evaluated the Head is now |
|
✅ Action performedFull review finished. |
Hermes follow-up charter review — exact head c67fc19Gate states
Adjudication of the former P1Resolved
Remaining P1
ResultThe original P1 is NOT fully resolved. Lane count and assessability accuracy are resolved, and the code/test preservation PASS carries forward. Two documentation blockers remain: one unresolved non-root-relative test-file citation, and the overbroad “append variants so older readers keep parsing” wire-format statement. The review reports gate states only; it is not a merge verdict. Coverage: exact PR head, the three follow-up commit diffs, both architecture documents, the cited Autopilot reducer lines and surrounding call paths, evidence type declarations/serde behavior, formatter baseline/workflow/blame disposition, and current PR checks/reviews. No files were modified. |
…st citation Two follow-up defects from the charter re-review, one of them mine. The additive-only cell claimed appending enum variants keeps older readers parsing. That is false for the plain derived enums in evidence.rs (IntuneTimestampKind, IntuneSensitivity, IntuneParseState, IntuneAccessState, IntuneArtifactStatus): none carries a #[serde(other)] arm, so an unknown variant is a hard decode error. Only intune_raw_preserving_string_enum! types have an Unknown(String) path. The crate already states the correct rule on INTUNE_EVIDENCE_SCHEMA_VERSION at evidence.rs:23-24, and the document contradicted it. The cell now separates the wire-format question from the Rust API question and matches the crate on both. The remaining non-root-relative citation (tests/intune_windows_microsoft_store.rs) is now prefixed. The verifier that was supposed to catch it only validated paths already beginning with crates/ or docs/, so a bare tests/ reference was invisible to the check written to find exactly that defect; the check is now prefix-agnostic. Documentation only; no code or test changes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Both remaining findings fixed at The wire-format overclaim was my error, and it's the important one. In fixing the original inaccurate guardrail I introduced a second inaccuracy: I wrote that appending enum variants keeps older readers parsing. That is false for the plain derived enums here. The crate already stated the correct rule on Citation — P2 (cargo fmt) — thanks for the independent confirmation, including the differing block count. Pre-existing and non-gating either way. @coderabbitai full review |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
gh pr view 543 --repo adamgell/cmtraceopen --json headRefOid,headRefName -q '.'
git fetch origin pull/543/head 2>&1 | tail -5 || true
git show 7d1150ab --stat 2>&1 | head -30 || true
echo "---evidence.rs schema comment---"
git show 7d1150ab:crates/cmtraceopen-parser/src/intune/evidence.rs 2>/dev/null | sed -n '1,40p'
echo "---shared-vs-workload doc wire format section---"
git show 7d1150ab:docs/architecture/shared-vs-workload-invariants.md 2>/dev/null | grep -n -i "wire" -A5 -B5
echo "---tests/ citation check---"
git show 7d1150ab:docs/architecture/shared-vs-workload-invariants.md 2>/dev/null | grep -n "intune_windows_microsoft_store.rs"
git show 7d1150ab:docs/architecture/decisions/ADR-001-evidence-strength-confidence.md 2>/dev/null | grep -n "tests/"Length of output: 7581
Wire-format claim. Line 25 of Citation. Line 162 now reads Both findings are resolved. The diff is docs-only, as stated. Note: this confirms document accuracy against the current source. It does not substitute for CI or a CodeRabbit approval on this head, both separate gates you already track in your own review. You are interacting with an AI system. |
Hermes final adjudication — exact head 7d1150aAdjudication of the two remaining blockers1. Wire-format compatibility claim — RESOLVEDI read
The former overbroad wire-format blocker is closed. 2. Repository-root citations — RESOLVEDI independently re-swept both architecture documents at the exact remote head, using the complete repository tree from
The only non-resolving tokens surfaced by the deliberately broad lexical scan were ordinary prose fragments ( The citation blocker is closed. Preservation and scope
Gate states
This is a gate-state report, not a merge verdict. Coverage was limited to the two requested blockers, the exact-head evidence declarations, both architecture documents, the follow-up diff, and current PR gate state; no files were modified and the branch was not checked out. |
There was a problem hiding this comment.
Pull request overview
This PR performs a thin “Framework v1” extraction in the Intune Windows reducer ecosystem, focusing on deduplicating a shared IntuneAccessState ↔ IntuneArtifactStatus mapping, consolidating repeated test-harness helpers, and documenting which invariants are shared vs intentionally workload-specific.
Changes:
- Centralizes the 7-arm bijective mapping between
IntuneAccessStateandIntuneArtifactStatusintocrates/cmtraceopen-parser/src/intune/evidence.rs(both directions) and moves/expands the exhaustiveness test alongside it. - Consolidates repeated Intune fixture test helpers into
crates/cmtraceopen-parser/tests/support/mod.rs(scenario roots, captureState mappings, JSON “wire” serialization, and sorted evidence-id extraction where order is not part of the contract). - Adds/updates architecture documentation to explicitly distinguish shared invariants from protected divergences and to clarify ADR-001’s “directional doctrine” as shared prose with workload-specific mechanisms.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| docs/architecture/shared-vs-workload-invariants.md | New catalogue of shared vs workload-specific invariants; documents protected divergences and extraction criteria. |
| docs/architecture/decisions/ADR-001-evidence-strength-confidence.md | Adds an ADR addendum clarifying shared doctrine vs per-lane implementation mechanisms. |
| crates/cmtraceopen-parser/tests/support/mod.rs | Adds shared test-support helpers (scenario root, captureState mappings, wire serialization, sorted evidence ids). |
| crates/cmtraceopen-parser/tests/intune_windows_win32.rs | Switches Win32 tests to shared captureState/access-state helpers and shared scenario-root helper. |
| crates/cmtraceopen-parser/tests/intune_windows_microsoft_store.rs | Switches Store tests to shared captureState→status helper; preserves local ordered evidence-id helper (documented). |
| crates/cmtraceopen-parser/tests/intune_windows_configuration.rs | Switches Configuration tests to shared captureState→status helper, scenario-root helper, and shared wire. |
| crates/cmtraceopen-parser/tests/intune_windows_compliance.rs | Switches Compliance tests to shared captureState→status helper, scenario-root helper, and shared sorted evidence ids helper. |
| crates/cmtraceopen-parser/tests/intune_windows_autopilot.rs | Switches Autopilot tests to shared scenario-root and shared wire; keeps lane-local captureState enum conversion (documented). |
| crates/cmtraceopen-parser/src/intune/evidence.rs | Introduces the shared mapping functions and a bijection test colocated with the enums. |
| crates/cmtraceopen-parser/src/intune/enrollment/windows/autopilot/sources.rs | Refactors Autopilot declared-status derivation to use the shared access-state→status mapping. |
| crates/cmtraceopen-parser/src/intune/apps/windows/win32/sources.rs | Replaces local access-state→coverage-status mapping with shared helper; relocates the related test. |
| crates/cmtraceopen-parser/src/intune/apps/windows/microsoft_store/reducer.rs | Replaces local status→access-state mapping with shared helper. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
|
||
| | Concern | Owner | Notes | | ||
| |---|---|---| | ||
| | Evidence envelope types (`IntuneObservationContext`, `IntuneEvidenceRef`, `IntuneProvenance`, `IntuneArtifactCoverage`, `IntuneFinding`, ...) | `crates/cmtraceopen-parser/src/intune/evidence.rs` | Two separate compatibility questions, and only one of them has an "additive" answer. **Wire format:** exactly as `INTUNE_EVIDENCE_SCHEMA_VERSION` states at `crates/cmtraceopen-parser/src/intune/evidence.rs:23-24` — only an optional field, or a variant on an `intune_raw_preserving_string_enum!` type, is additive. The plain `#[derive(Deserialize)]` enums (`IntuneTimestampKind`, `IntuneSensitivity`, `IntuneParseState`, `IntuneAccessState`, `IntuneArtifactStatus`) have no `#[serde(other)]` arm, so an unknown variant is a hard decode error: adding one breaks older readers and bumps the schema version. **Rust API:** no additive guarantee at all. None of these types are `#[non_exhaustive]` and `cmtraceopen-parser` is published, so a new variant or public field breaks downstream `match` arms and struct literals and needs a semver-major release, or `#[non_exhaustive]` first. | |
Thin PR 4 of the Reducer Framework. An inspection cycle concluded that almost nothing should be extracted; this implements exactly the three items that survived that conclusion. No reducer behavior changes.
1. Extracted: the
IntuneAccessState<->IntuneArtifactStatusmappingThree copies of a pure 7-arm total bijection existed. Every arm was verified identical before replacing (see "arm-by-arm verification" below). Both directions now live in
crates/cmtraceopen-parser/src/intune/evidence.rs, beside the two enums:artifact_status_for_access_state(&IntuneAccessState) -> IntuneArtifactStatusaccess_state_for_artifact_status(&IntuneArtifactStatus) -> IntuneAccessStateBoth are exhaustive
matchwith no_arm, so a new variant on either enum is a compile error rather than a silently defaulted status in whichever lane copied the table last. Signatures take&because neither enum isCopyand addingCopywould be an unrelated public API change.The exhaustiveness test moved from
win32/sources.rstoevidence.rsand now also pins the inverse direction (the Store lane's half), which nothing tested before.Lane updates:
apps/windows/win32/sources.rscoverage_statusartifact_status_for_access_stateapps/windows/microsoft_store/reducer.rsaccess_state_foraccess_state_for_artifact_statusenrollment/windows/autopilot/sources.rsdeclared_statusandaccess_state, both 7-arm tables over the lane-localAutopilotCaptureStateaccess_statekept (it is this lane's own collector vocabulary);declared_statusis nowartifact_status_for_access_state(&self.access_state())Autopilot keeps
access_statebecause its source enum isAutopilotCaptureState, which is not shared and should not be. Only the second hop,IntuneAccessState -> IntuneArtifactStatus, was ever the shared part.Arm-by-arm verification
IntuneAccessStateIntuneArtifactStatuscoverage_statusaccess_state_for(inverse)declared_status/access_stateAvailableAvailableCaptured-> both, consistentMissingMissingAbsent-> both, consistentPermissionDeniedPermissionDeniedAccessDenied-> both, consistentCappedCappedCapped-> both, consistentSkippedSkippedSkipped-> both, consistentFailedParseFailedParseFailed->ParseFailed/Failed, consistentUnsupportedUnsupportedUnsupported-> both, consistentNo lane had a divergent arm.
declared_status(x) == artifact_status_for_access_state(&access_state(x))held for all seven Autopilot capture states, which is why the composition is a pure dedup and not a normalization.2. Mechanical test-support consolidation
Added to
crates/cmtraceopen-parser/tests/support/mod.rs:scenario_root(corpus, scenario)— replaces 5 local copies (win32 spelled itcorpus().join(...)).artifact_status_for_capture_state(&str) -> IntuneArtifactStatus— replaces 3 local copies (microsoft_store::status_for,compliance::capture_status,configuration::status_for), which were byte-identical.access_state_for_capture_state(&str) -> IntuneAccessState— the Win32 adapter. Implemented as the status table composed with the crate's ownaccess_state_for_artifact_status, rather than a second hand-written table, so a fixture can never disagree with the crate about whatparseFailedmeans.wire<T: Serialize>(&T) -> Value— replaces the identicalconfiguration::wireandautopilot::wire(one generic, oneimpl Trait).sorted_evidence_ids(&Value) -> Vec<String>— explicitly named, for consumers where citation order is not part of the contract.configuration::status_forcarried a comment claiming it was "kept explicit rather than derived from the harness table so a drift in either direction is a compile-visible change here". That rationale was aimed at theCAPTURE_STATE_COVERAGEstring-pair table, not at a typed helper; the typed helper it now calls is strictly more drift-resistant. Flagging it because it was an explicit anti-consolidation note and reviewers should agree with removing it.evidence_idsdetermination, per consumerInvestigated before touching either copy. They are not two spellings of one helper; they assert different contracts.
intune_windows_compliance.rs—assert_settings(settings[].evidence)evidence_idssorted, and it was paired againststrings(&want[...])which also sorts. Both sides normalized, so no assertion could ever observe order.intune_windows_compliance.rs—assert_access(access[].matchedEvidence)strings(sorted).intune_windows_compliance.rs—assert_findings(findings[].evidence)strings(sorted). Note the siblingcoverageGapIdsassertion in the same function isstringsvsstrings, i.e. the whole leaf is consistently set-valued.intune_windows_microsoft_store.rs—assert_transactions(transactions[].evidence)evidence_idspreserves insertion order and is paired againstexpected_strings, which does not sort, soassert_eq!on twoVec<String>asserts order directly. Immediately below it, the same function assertsobservations.len() == evidence.len()with the message "observations and evidence must stay in step" — a positional pairing between the two arrays. Sorting the evidence side would silently retire that pairing with no test failing.intune_windows_microsoft_store.rs—assert_findings(findings[].evidence)expected_stringscounterpart.So: compliance moved to
support::sorted_evidence_ids(all three call sites); Microsoft Store kept its local version, now carrying a doc comment stating why it is deliberately not the shared helper. The two were not forced onto one helper.The store copy also panics where compliance's defaults to
""on a missingevidenceId. That difference is preserved too: in the store corpus a missingevidenceIdis a corpus bug, and defaulting it would let a broken fixture compare equal.Deliberately NOT extracted (one line each)
capture_state(test helper) — returns the lane-localAutopilotCaptureStatevia serderename_all, notIntuneArtifactStatus; routing it through the shared helper would mean re-deriving a lane enum from a status, which is backwards.AutopilotCaptureState::access_state— the collector-vocabulary hop is genuinely Autopilot's to own; only the second hop was shared.evidence_ids— asserts citation order and positional step withobservations; see the table above.Cappedas able to prove a terminal outcome while Autopilot/Compliance do not; the predicates contradict each other by contract.Low, Win32 toMedium; contradictory by contract, both correct.activity_idpairwise, Autopilot orderlessactivity_idsets, Win32 two grammars plus transitive closure, Configuration refuses and returnsContested); a shared helper would have to be the loosest of the four.CandidateOrder, evidence-resolution assertions — out of scope for this PR by instruction; untouched.3. Documentation
docs/architecture/decisions/ADR-001-evidence-strength-confidence.md— addendum: the directional doctrine ("non-assessable evidence cannot prove, but a recorded non-assessable failure must still block a success") is shared prose, its mechanism is per-lane. Cites both sites: Autopilotenrollment/windows/autopilot/reducer.rs~650-707 (recorded_non_assessable_failure_sections/..._observations) enforced ~1668-1688, and Configurationdevice/windows/configuration/sources.rs~163-174 (is_unassessable_failure) enforced in.../configuration/reducer.rs~246-263.docs/architecture/shared-vs-workload-invariants.md(new) — a table of what is shared vs deliberately workload-specific, explicitly protecting the tested divergences so a future "consistency" PR cannot delete them: the Win32 capped-but-terminal rule and its test, Autopilot's ungatedtime_basisand itsAutopilotKeyGate::Detecting/Provingsplit, Configuration admittingIntuneParseState::Raw, the Store-vs-Win32 degradation constants, the four linkage contracts, and the redaction grammar-vs-projection line. Closes with a three-part extraction test.Gates
cargo test -p cmtraceopen-parsercargo clippy -p cmtraceopen-parser --all-targets -- -D warningscargo check --workspacecargo check -p cmtraceopen-parser --target wasm32-unknown-unknowngit diff --checkNo
tauri::test::mock_app(). No rustfmt run on files not created in this PR.Defects found
None. No lane had a divergent arm, and nothing required a RED-first follow-up.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Documentation
Tests