refactor(intune): ask the citation predicate instead of restating it in two lanes - #548
Merged
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>
The compliance reducer folded observations in raw caller order and never canonicalized them. Six merges read vector position rather than record content, and one of them flipped an exported conclusion: phase.state alternated between Submitted and Stale when two submissions straddled the latest local evaluation, because last_submission_at was an unconditional last-wins assignment feeding the staleness gate. Every field now merges by content, following the template Configuration already ships: smallest stated value, agreed value (None/Unknown on an unresolved disagreement, per ADR-003), and terminal error by canonical code ordering. last_submission_at is the latest submission by timestamp and refuses to answer when several submissions cannot all be ordered. Also closed in the same class: the weaker report-status tokens folded by last-writer-wins rather than by rank, the prerequisite detail followed the last unmet row, and the three exported fact vectors kept the caller's order in the serialized snapshot. No fixture expectation changed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…RED) These tests fail. They are committed before the fix so the defect is readable from history rather than asserted after the fact. `docs/architecture/shared-vs-workload-invariants.md` divergence 7 states that the masking grammar has exactly one owner (`intune/apps/windows/common/redaction.rs::redact_text`) and that each lane owns only its projection. The Compliance lane carried a private copy of the grammar. The copy has drifted, and every drift leaks: - short SID: exported "S-1-5-21-1010" verbatim - legacy profile root: exported "Jane Roe" verbatim - JSON-escaped profile path: exported "Jane Roe" verbatim - device name field: exported "DESKTOP-AB12CD" verbatim - UNC server segment: exported "FILESRV01" verbatim - account field: exported "jsmith" verbatim - tenant id field: exported the tenant GUID verbatim - inline credential flag: exported "hunter2" verbatim - MSI property credential: exported "ABCD-1234-EFGH" verbatim The third test pins the export surface itself, not just the helper: a SID placed in a setting display name survives into the serialized redacted projection. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Makes the nine RED assertions from the previous commit pass by deleting
the fork rather than patching the one reported pattern.
The reported defect was the SID rule: Compliance required four
sub-authorities where the owner requires three, so `S-1-5-21-1010` -- a
SID that names a real principal -- reached the export verbatim while
every sibling lane masked it. Auditing the rest of the lane's grammar
found the SID rule was not special. The fork was missing or weaker on
eight further rules, and the direction was never once conservative:
pattern owner Compliance fork
SID >= 3 sub-authorities >= 4 (leaked short SIDs)
profile path Users + legacy root, Users only, single
1-2 separators separator (leaked JSON-
escaped and legacy paths)
device name field masked absent
UNC server segment masked absent
account field masked absent
tenant id field masked absent
inline credential masked absent
MSI property masked absent
UPN masked identical (only match)
Compliance is the lane that can least afford this: a custom-compliance
script prints whatever its author decided to print, and that output is
free text this grammar is the only thing standing in front of.
So the fix is structural. `redaction.rs` now re-exports
`intune::apps::windows::common::redact_text` -- the single owner named in
`docs/architecture/shared-vs-workload-invariants.md` divergence 7, and
the same arrangement win32, scripts and remediations already use -- and
keeps only what is genuinely this lane's: the projection deciding which
compliance fields are sensitive. Tightening the nine patterns in place
would have fixed today's drift and left the mechanism that produced it.
The four grammar tests in this module move to the owner's test module
rather than being deleted; all four cases were already pinned there. The
lane's own obligation is now asserted as byte-for-byte output parity with
the owner, which is strictly stronger and fails on any future fork.
No fixture expectation changed. The golden in
`deterministic-privacy-redaction/expected.json` still matches byte for
byte, because the two grammars hash identical spans to identical tokens
wherever they both fired -- the divergence was entirely about spans the
fork did not fire on at all.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…lanes Both lanes hand-inline the De Morgan negation of the shared invariant IntuneFinding::is_evidence_backed inside their private finding() constructors. Before touching either, pin what they do today. The guard reads exactly two bits (is the evidence vector empty, is the coverage-gap vector empty) and no lane-specific state at all, so the four cells asserted here are the complete input space. Suppression is asserted against the shared predicate rather than restated, so the two can only agree. The tests also pin what is NOT shared between the two constructors: Autopilot sorts and de-duplicates both citation sets before deciding (emptiness-preserving, so the verdict is unchanged, but it is this lane's byte-identical output contract), while Compliance keeps its citations verbatim and takes a single recommended check rather than a slice. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The Autopilot and Compliance finding constructors each hand-inlined the De Morgan negation of IntuneFinding::is_evidence_backed while their own comments named that predicate as the invariant they enforce. Both now build the candidate and ask it, the way the Win32, Microsoft Store and Configuration lanes already do at their own choke points. Behavior-preserving, proved on the complete input space rather than by shape. The guard reads two emptiness bits and no lane-specific state, so the space is four cells, and suppression agrees with the predicate on all four; the pinning tests from the previous commit assert exactly that and are unchanged here. Autopilot's normalization is emptiness-preserving (sort never changes length, dedup never empties a non-empty vector), so moving the question after it cannot change the verdict. What is NOT unified: the constructors themselves. Autopilot normalizes both citation sets and takes a slice of recommended checks; Compliance keeps citations verbatim and takes one check. The ESP lane carries the same shape over EspDiagnosticFinding, a separate type family that cannot call this predicate at all. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…oes not The verdict "does this finding cite anything" has one owner and now one caller shape per lane, so it joins the "actually shared" table. The constructors that ask it do not, and that is the part a future consistency PR is likely to reach for next: three shapes across five lanes, disagreeing on normalization, on the arity of recommended_checks, and on whether a side effect is ordered against the gate. Divergence 9 records why the verdict passed the extraction test while the constructors fail part 2 of it, and why the ESP lane's identical-looking constructor is out of scope over a separate type family. Every citation is repository-root-relative with a line number checked against the line it names. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
github-actions
Bot
added
enhancement
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe change updates Compliance and Autopilot finding constructors to use ChangesFinding validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
# Conflicts: # crates/cmtraceopen-parser/src/intune/device/windows/compliance/reducer.rs # crates/cmtraceopen-parser/tests/intune_windows_compliance.rs # docs/architecture/decisions/ADR-001-evidence-strength-confidence.md # docs/architecture/shared-vs-workload-invariants.md
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/shared-vs-workload-invariants.md`:
- Around line 199-203: Update docs/architecture/shared-vs-workload-invariants.md
lines 199-203 by replacing every lane’s placeholder “.../” path with the full
repository-relative crates/cmtraceopen-parser/src/... path for the referenced
constructors. Also update lines 221-226 by replacing the Autopilot and
Compliance test-reference placeholders with their full repository-relative
paths, ensuring all documented paths exist.
🪄 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: 2199ec89-d147-4215-a7f2-c3cd965b6cb5
📒 Files selected for processing (3)
crates/cmtraceopen-parser/src/intune/device/windows/compliance/rules.rscrates/cmtraceopen-parser/src/intune/enrollment/windows/autopilot/rules.rsdocs/architecture/shared-vs-workload-invariants.md
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors the Intune Windows Autopilot and Compliance lanes to delegate the “citation verdict” to the shared IntuneFinding::is_evidence_backed predicate (instead of inlining its negation), adds per-lane tests pinning guard behavior across the full 4-cell input space, and documents that the predicate is shared while constructor shapes remain intentionally lane-specific.
Changes:
- Replace the Autopilot/Compliance inline “no citations => suppress” guard with
candidate.is_evidence_backed().then_some(candidate). - Add constructor-focused unit tests in both lanes to pin guard semantics and lane-specific normalization/shape differences.
- Update the shared-vs-divergent architecture doc to explicitly list the citation verdict as shared and record constructor divergence.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| docs/architecture/shared-vs-workload-invariants.md | Documents the shared citation verdict and the protected divergence around per-lane finding constructors. |
| crates/cmtraceopen-parser/src/intune/enrollment/windows/autopilot/rules.rs | Uses the shared predicate for the citation guard and adds tests covering the 4-cell input space plus normalization behavior. |
| crates/cmtraceopen-parser/src/intune/device/windows/compliance/rules.rs | Uses the shared predicate for the citation guard and adds tests covering the 4-cell input space plus constructor shape guarantees. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
683
to
+697
| @@ -694,7 +693,8 @@ fn finding( | |||
| recommended_checks: vec![check.to_owned()], | |||
| evidence, | |||
| coverage_gap_ids, | |||
| }) | |||
| }; | |||
| candidate.is_evidence_backed().then_some(candidate) | |||
Comment on lines
899
to
+912
| let mut coverage_gap_ids = coverage_gap_ids; | ||
| coverage_gap_ids.sort(); | ||
| coverage_gap_ids.dedup(); | ||
| if evidence.is_empty() && coverage_gap_ids.is_empty() { | ||
| return None; | ||
| } | ||
| Some(IntuneFinding { | ||
| let candidate = IntuneFinding { | ||
| finding_id: id.to_owned(), | ||
| severity, | ||
| confidence, | ||
| title: title.to_owned(), | ||
| summary: summary.to_owned(), | ||
| recommended_checks: recommended_checks.to_vec(), | ||
| evidence, | ||
| evidence: normalized_evidence(evidence), | ||
| coverage_gap_ids, | ||
| }) | ||
| }; | ||
| candidate.is_evidence_backed().then_some(candidate) |
The divergence-9 section used '.../lane/file.rs' placeholders, which do not identify a file from the repository root. All are now repository-relative with the line number in path:line form. Two further citations did not resolve and were not introduced here: the divergence-7 entry added in #546 cites intune/device/windows/compliance/ redaction.rs and tests/intune_windows_compliance.rs without the crate prefix. They are on main now, so they are corrected here rather than left for a separate PR. The check that was supposed to catch all of this only validated paths already beginning with crates/ or docs/, so both an abbreviated '.../' path and a crate-relative one were invisible to it. It is now prefix-agnostic and also rejects any remaining '.../'. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Owner
Author
|
Fixed at The sweep also found two further citations that do not resolve and were not introduced here: the divergence-7 entry added in #546 cites Worth recording why they survived: my own citation check only validated paths already beginning with Verified: every path-shaped citation in the document resolves from the repository root, no abbreviations remain, and the cited line numbers point at what the prose claims. @coderabbitai full review |
|
✏️ Learnings added
✅ Action performedFull review finished. |
adamgell
added a commit
that referenced
this pull request
Aug 11, 2026
Revision 1 was written as a proposal with six open questions and a recommendation attached to each. The owner has ruled, so the document now states decisions rather than options. The six questions become eight recorded rulings, each carrying the reasoning that supports it, one line on what a lane author must now do differently, and the mechanism that catches a violation (compiler, test, or review). Two rulings refine what the draft recommended and say so explicitly: - Ruling 3 replaces the draft's caller-supplied "analysis identity" with an opaque caller-owned RedactionContext. The crate does not interpret the value, derive meaning from it, or require it to name anything. This dissolves the draft's open question about which identity a caller should pass, and makes every testable property relational rather than semantic. - Ruling 4 forbids the fallback the draft left unruled. Configuration degrades its scope to generatedAtUtc alone when no scope is supplied. A lane with no context must now decline or emit with no equality; it may not substitute material that travels with the export. Ruling 6 sharpens Restricted from "a masked value" to no value-derived representation at all, enforced by one shared test asserting that two differing restricted inputs produce identical output. Ruling 7 states exhaustive construction recursively and splits the enforcement honestly: the compiler catches a new field only once the constructor is exhaustive, so the construction style itself is a review obligation. Re-verified against origin/main at f174012, which moved three of the draft's claims. Compliance no longer carries a private masking grammar (#546 deleted the fork; the lane re-exports the shared owner and pins byte-for-byte parity), its structural citations moved, and its defect is the inverse of the shape the draft described: non-exhaustive at the top level, exhaustive in a nested view. The grammar-owner row in shared-vs-workload-invariants.md also moved (#548). Compliance's private unsalted minter did survive, which is a useful demonstration that sharing the grammar and sharing the derivation are independent questions. Scope is unchanged and contract-only: no primitive named, no migration plan, no source file touched. The security requirement stays a property. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
adamgell
added a commit
that referenced
this pull request
Aug 13, 2026
…pe and ownership (#550) * docs(adr): propose ADR-004 revision 1 (redaction contract scope and ownership) A decision document, not an implementation. It decides what the redaction contract promises and who owns each part of it, and decides nothing about how. Six rulings, each independently acceptable, amendable, or rejectable: 1. Where the contract binds: the crate/library API, generalizing the two lanes that already bind by construction. IPC/emit deferred; frontend egress explicitly out of scope as a boundary. 2. Equality scope: one analysis, caller-supplied identity. The security requirement is stated as a property, not an algorithm. 3. Ownership: grammar shared, projection per-lane, derivation shared, with a test for the distinction rather than a list. 4. Restricted vs Sensitive: give Restricted a real distinct behaviour rather than removing it or leaving it decorative. 5. Structural obligations: exhaustive construction enforced recursively by the compiler; self-description by type where the lane can express it. 6. Cross-lane correlation: out of scope, stated plainly, with a prohibition on presenting a shared derivation as delivering it. Token mechanics and migration are deliberately deferred. No source file changes. Records a direct contradiction inside the existing architecture: ADR-004 forbids stable cross-artifact correlation tokens and five of six Windows Intune lanes mint exactly those, with global stability stated as the design goal in their own module docs. Every code reference was opened and verified against origin/main at 2678f1f. Five inventory claims were found to be wrong or imprecise and are corrected in the document rather than silently applied, including that SCCM already projects by construction on its export path. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(adr): record the owner's eight rulings on ADR-004 revision 1 Revision 1 was written as a proposal with six open questions and a recommendation attached to each. The owner has ruled, so the document now states decisions rather than options. The six questions become eight recorded rulings, each carrying the reasoning that supports it, one line on what a lane author must now do differently, and the mechanism that catches a violation (compiler, test, or review). Two rulings refine what the draft recommended and say so explicitly: - Ruling 3 replaces the draft's caller-supplied "analysis identity" with an opaque caller-owned RedactionContext. The crate does not interpret the value, derive meaning from it, or require it to name anything. This dissolves the draft's open question about which identity a caller should pass, and makes every testable property relational rather than semantic. - Ruling 4 forbids the fallback the draft left unruled. Configuration degrades its scope to generatedAtUtc alone when no scope is supplied. A lane with no context must now decline or emit with no equality; it may not substitute material that travels with the export. Ruling 6 sharpens Restricted from "a masked value" to no value-derived representation at all, enforced by one shared test asserting that two differing restricted inputs produce identical output. Ruling 7 states exhaustive construction recursively and splits the enforcement honestly: the compiler catches a new field only once the constructor is exhaustive, so the construction style itself is a review obligation. Re-verified against origin/main at f174012, which moved three of the draft's claims. Compliance no longer carries a private masking grammar (#546 deleted the fork; the lane re-exports the shared owner and pins byte-for-byte parity), its structural citations moved, and its defect is the inverse of the shape the draft described: non-exhaustive at the top level, exhaustive in a nested view. The grammar-owner row in shared-vs-workload-invariants.md also moved (#548). Compliance's private unsalted minter did survive, which is a useful demonstration that sharing the grammar and sharing the derivation are independent questions. Scope is unchanged and contract-only: no primitive named, no migration plan, no source file touched. The security requirement stays a property. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(adr): stop overstating what revision 1 decides Revision 1 resolves the equality scope and the caller-owned context, not the key, algorithm, encoding, secret source, or cross-export behaviour. The status notices, the frontend projection claim, the token-equality biconditional, the grammar-ownership exception, the invariant table, the migration bullet, and the cross-export answer are all restated to match what the rulings actually decide, and the original ADR's notice and status no longer describe equality scope as both resolved and provisional. --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
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.
What was reported
The Autopilot and Compliance lanes were reported as each carrying their own
is_evidence_backedpredicate.What is actually there
The premise is half right. There is exactly one
is_evidence_backedin thetree (
crates/cmtraceopen-parser/src/intune/evidence.rs:355). What the twolanes carried was the hand-inlined De Morgan negation of it, inside their
private
finding()constructors:The other three Windows lanes (Win32, Microsoft Store, Configuration) already
called the shared predicate at their own choke points.
Arm-by-arm comparison
The guard reads exactly two bits and no lane-specific state at all: it never
consults the snapshot, access state, parse state, severity, or confidence. So
the input space is four cells, and it is exhaustive.
evidencecoverage_gap_idsNone(suppressed)None(suppressed)falseSomeSometrueSomeSometrueSomeSometrueSuppression agrees with the negation of the shared predicate on every cell.
Autopilot normalizes (
normalized_evidence, plussort/dedupon the gapids) before deciding. That is safe rather than lucky:
sortnever changesa vector's length and
dedupnever empties a non-empty one, so normalizationpreserves both emptiness bits and cannot move the answer.
Verdict
The verdict is identical, and identical for the same stated reason. Each
lane's own doc comment already named
IntuneFinding::is_evidence_backedas theinvariant it was enforcing, so the agreement is not coincidental, and a lane
emitting an uncited finding would violate the invariant declared on
IntuneFindingitself. That is all three parts of the doc's extraction test.The constructors hosting it are deliberately per-lane and are NOT unified
here. Autopilot normalizes both citation sets and takes
recommended_checks: &[String]; Compliance keeps citations verbatim and takesa single
check: &str; Win32 runs the redaction grammar oversummaryafterthe gate; Microsoft Store takes
&[&str]. A shared constructor would have topick one normalization policy and would either impose Autopilot's
byte-identical ordering contract on Compliance or retire it from Autopilot.
The ESP lane carries the same constructor shape at
src/esp/rules.rs:669overEspDiagnosticFinding/EspEvidenceRef, a separate type family that cannotcall this predicate at all. Left alone.
What changed
Three commits, in this order:
test(intune)pins the current behavior of both lanes across the fullfour-cell space, plus what is not shared between the constructors
(Autopilot sorts and dedupes; Compliance preserves order and duplicates and
wraps its single check).
refactor(intune)replaces the two inlined negations withcandidate.is_evidence_backed().then_some(candidate). The pinning tests areunchanged by this commit and still pass.
docs(intune)adds the verdict to the "actually shared" table and addsprotected divergence 9 covering the constructors, so the next consistency PR
stops at the predicate. All eleven citations are repository-root-relative
and were checked line by line against what the prose claims.
No behavior change rides along. No
expected.jsonfixture is touched. Nogenuine defect was found during the work.
Gates
Stacked on
fix/compliance-sid-redactionbecause PRs #545 and #546 alreadyrewrote
tests/intune_windows_compliance.rson that chain.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Documentation