Skip to content

fix(intune): apply framework review discipline to the merged Autopilot lane - #531

Merged
adamgell merged 14 commits into
mainfrom
autopilot/framework-hardening
Aug 9, 2026
Merged

fix(intune): apply framework review discipline to the merged Autopilot lane#531
adamgell merged 14 commits into
mainfrom
autopilot/framework-hardening

Conversation

@adamgell

@adamgell adamgell commented Aug 8, 2026

Copy link
Copy Markdown
Owner

Applies Reducer Framework v1 review discipline (docs/superpowers/plans/2026-08-07-reducer-framework-v1.md, ADR-001..004) to the merged Autopilot lane. Input was the full set of PR #450 review threads; every finding was verified against the merged code, then fixed, rejected with reasoning, or deferred with reasoning. Each real defect got a failing test before its fix, and every corrected golden documents in its assertions why the prior expectation was unsafe.

Refs #362. Follow-up to PR #450.

Triage of all 20 review findings

# Finding (thread) Severity Verdict ADR / reasoning
1 reduce_profile skips the assessability gate (reducer.rs) Major Fixed ADR-001. A capped/unparsed success record could set retrieved/applied and reach Completed while the failure branch was filtered. Gated, plus class audit below.
2 Identity evidence bypasses the gate and inflates the phase (reducer.rs) Minor Fixed ADR-001, same class. An unreadable record alone raised the phase to IdentityObserved.
3 Conflicting ESP linkage can still report Completed (reducer.rs) Major Fixed ADR-003. The multi-session match path records no AutopilotConflict, so the empty-conflicts gate let it fall through to Completed. reduce_outcome now matches Conflicting explicitly and returns ContradictoryEvidence; a new autopilot-esp-link-conflicting finding explains exactly the previously-silent path.
4 matched_keys masking skips trim/lowercase normalization, 4 sites (redaction.rs) Major Fixed ADR-004. All four whole-value sites now call mask_value; regression test covers mixed case/whitespace across named data, conflict values (both shapes), matched keys, and identity fields.
5 Schema-version-only capture yields UnknownSchema with no finding (rules.rs) Minor Fixed ADR-001 (withheld semantics must still be explained). Gate now covers autopilot_schema_version; summary names whichever declared value failed validation.
6 windows_zone_re accepts placeholder junk as a declared timezone (sources.rs) Major Fixed ADR-002. unknown / not recorded / Unavailable classified as Declared, upgrading time basis and enabling the forbidden time-only ESP candidate. Regex now anchors on the Time suffix every Windows zone id ends with.
7 Golden regenerator races the readers of the same files (test harness) Major Fixed update_findings_golden is now #[ignore]d (run alone via -- --ignored), and write_json writes through a temp file + rename. Doc comments updated.
8 Malformed-report fixture is XML, never reaches the report-payload contract Minor Fixed Fixture now carries a correctly tagged report document with a mangled sections payload, exercising absorb_payload's malformed branch. Documented in the scenario's assertions.
9 Golden pins unstable serde_json error Display text Trivial Fixed Parse-failure details are now stable reducer-authored sentences at every site (serde_json does not guarantee Display stability); raw bytes are still retained as evidence.
10 Fixture comment claims "happy path" in completed-without-esp-bundle Minor Fixed Comment now describes the handoff-without-ESP-evidence scenario.
11 user-driven-success manifest claims a complete deployment Minor Fixed Description now claims local Autopilot phase completion only; post-handoff state belongs to ESP.
12 Comment overstates detect_conflicts coverage vs single_value Trivial Fixed (comment) Comment now names the three keys actually reported. The "shared key list" alternative was rejected: keys like deviceName legitimately change mid-provisioning, so promoting every single_value key to a conflict would fabricate contradictions.
13 No scenario exercises the native-event input path Trivial Fixed New test pins absorb_native_event: artifact derived from the event's own provenance, no document report, no coverage entry.
14 profileApplied set by event 153 ProfileState_Available is not application evidence Major Rejected The code wins over the review text. Event 172, the documented failure sibling, reads "failed to set Autopilot profile as available": setting the profile available IS the application step, so the 153 transition into Available is its explicit success record. Reasoning now recorded as a comment at the site in reduce_profile.
15 Add #[serde(default)] to NormalizedWindowsEvent.event_version or v1 docs fail Major Rejected serde deserializes a missing Option<T> field as None without any attribute. Empirical proof: the module doctest omits eventVersion entirely and passes in CI and locally.
16 error_code_from_token loses the upper 32 bits of 64-bit HRESULTs Major Rejected 0xFFFFFFFF80070002 is 0x80070002 sign-extended through a 64-bit register; canonicalizing the derived hex to 32 bits is the intended normalization, the raw token survives verbatim, and a genuinely 64-bit value gains no fabricated 32-bit hex. Behavior is now pinned by a_sign_extended_hresult_canonicalizes_to_its_32_bit_form.
17 Mark closed public enums and input/output structs #[non_exhaustive] Major Rejected (family convention) Checked the sibling lanes as the deciding factor: microsoft_store (10 public enums), compliance (13), esp, and the rest of the Intune family use zero #[non_exhaustive] (the only use in the crate is one SCCM type). The family's additivity strategy is the raw-preserving string-enum macro (unknown wire values survive verbatim in Unknown(String) without a new variant) plus the snapshot schema_version discipline documented in models.rs. Diverging on one lane would break exhaustive matching for in-repo consumers without protecting anything the macro does not already protect. If the crate later adopts #[non_exhaustive], it should be a family-wide decision, not an Autopilot-only one.
18 NormalizedWindowsEvent.event_version is a breaking API change needing a version bump + Rustdoc Major Deferred normalized.rs is a shared model owned by the parser-family skeleton and is explicitly out of scope for this lane-hardening PR (no shared-model changes). The field already shipped in merged main; releasing it under the crate's versioning process is a release-management concern for the family owner, not something to relitigate from one lane.
19 unknown-windows-schema-version cannot exercise the schema-only gate Trivial Fixed (via test) Covered by the new inline scenario a_schema_version_only_unknown_schema_is_explained_by_a_finding (windows build undeclared, schema version 3) rather than by widening the pinned 15-scenario fixture matrix, which issue #362 treats as a contract.
20 Fixture expectation churn from #5/#6/#9 Fixed + documented Both regenerated goldens (malformed-report-section, unknown-windows-schema-version) document in their assertions why the prior expectation was unsafe.

Class audit: which reduction paths gained the assessability gate

Beyond the two named findings, every reduction path in the module was audited (close the class, not the instance):

  • reduce_profile main observation loop — gained the gate (Major finding)
  • reduce_identity IDENTITY_KEYS evidence loop — gained the gate (Minor finding)
  • sections_ofgained a section-level gate (is_assessable_section), covering every report-section path at one enforcement point: identity sections, profile retrieval/application sections, handoff sections, and all four reduce_outcome section probes
  • detect_conflicts mismatch-section loop — gained the gate (an unreadable section cannot assert a mismatch)
  • autopilot_keys correlation-key extraction — gained the gate (a non-assessable record cannot mint a Linked correlation)
  • reduce_esp_linkage evidence-mention loop — gained the gate
  • has_time_overlapgained the gate (non-assessable timestamps cannot create a TimeOnlyCandidate)
  • signal_observations / has_signal / distinct_values (hence single_value) — already gated
  • time_basisdeliberately left unfiltered and documented: there the unfiltered scan is the conservative direction (a non-assessable record with an unnormalized timestamp downgrades the basis; gating it would upgrade it)

Gates (exact commands, run at head)

cargo test -p cmtraceopen-parser
  -> 2131 passed; 0 failed; 1 ignored (the #[ignore]d golden regenerator)
cargo clippy -p cmtraceopen-parser --all-targets -- -D warnings
  -> clean, 0 warnings
cargo check --workspace
  -> clean
cargo check --locked -p cmtraceopen-parser --target wasm32-unknown-unknown
  -> clean

Golden regeneration was run in isolation exactly once (UPDATE_AUTOPILOT_FINDINGS=1 cargo test --test intune_windows_autopilot -- --ignored update_findings_golden) and the diff reviewed; only the two summaries listed above changed.

Fix round (consolidated multi-agent review)

All three must-fixes and all five should-fixes from the consolidated review are addressed at head. TDD throughout: every behavioral item landed as a failing test first.

Must-fix 1, direction-aware assessability gate (a4d0daeb). sections_of stays assessable-only for everything that can prove progress or a terminal cause; the new recorded_non_assessable_failure_sections iterator routes an explicitly recorded failed/mismatch outcome on a capped/unparsed section to the success branch of reduce_outcome, which now returns InsufficientEvidence instead of Completed/HandoffReachedEspEvidenceMissing. Rationale documented in-code: ADR-001 forbids terminal conclusions from non-assessable evidence in both directions, so the failure outcome itself stays reserved for assessable records and the honest reduction is InsufficientEvidence (Low confidence), never a silent success. Surfaced by the new autopilot-non-assessable-failure-recorded finding (warning, low confidence, cites the section, asks for a readable re-collection). NotFound/Retrying stay gated in both directions: absence statements from a partial capture prove nothing. Fixtures: a_recorded_failure_on_a_non_assessable_section_blocks_success (assessable success path + capped profileApplication failed section, the exact scenario the review named), plus the negative control an_assessable_failed_section_still_produces_the_terminal_failure.

Must-fix 2, correlation keys survive for conflict detection (6c031f92). autopilot_keys is now gated by AutopilotKeyGate::{Proving, Detecting}: proving keys (assessable only) remain the only ones able to produce Linked and the Completed behind it; detecting keys (all observations) feed a multi-session conflict check that runs before any positive linkage and returns Conflicting naming every detected session. More sessions detected means more conservative; a non-assessable-only match can never upgrade past NotObserved/TimeOnlyCandidate. Fixtures: a_key_on_a_capped_observation_still_detects_a_second_esp_session (session B matched only via a capped observation, so no silent 2-to-1 collapse) and a_non_assessable_only_key_match_cannot_upgrade_to_linked.

Must-fix 3, matched_keys masking pinned as a contract (cca27db8). matched_key_masking_normalizes_case_independently_of_the_reducer pins both halves: autopilot_keys hands the projection lowercase values, and the redaction.rs masking loop still produces the same token when fed a mixed-case value it never normally receives.

Should-fix 4, case-insensitive distinct_values (5ce8e2e5). Decided for case-insensitivity: serials/GUIDs are case-insensitive identities and the export masks the trimmed lowercased value, so a case-only difference exporting "2 distinct values" over two identical tokens changed a conclusion under redaction (ADR-004). Groups now key on the lowercased value with a deterministic representative (lexicographically smallest sighting, permutation-safe per ADR-003). Tests: a_case_only_identifier_difference_is_not_a_conflict + control genuinely_distinct_identifiers_still_conflict.

Should-fix 5, triage bookkeeping (5ce8e2e5). The 153/172 application-evidence reasoning is now in the assertions array of the matching-autopilot-and-esp-session golden. Bookkeeping correction for the triage table: the opaque_blob_re doc-comment finding was fixed in this PR but was omitted from the table; row 20 being self-authored, the honest count is 19 externally reported + 1 self-reported.

Should-fix 6, evidence request survives the time gate (5ce8e2e5). The shared-identifier next_evidence_request is now keyed on "ESP facts supplied but not explicitly linked" (NotObserved or TimeOnlyCandidate), so the narrowed assessable overlap window can no longer erase the guidance. Test: unlinked_esp_sessions_keep_the_shared_identifier_evidence_request.

Should-fix 7, windows_zone_re (5ce8e2e5). Comment corrected (no more "every Windows id ends in Time"; UTC/UTC+12 are registry ids handled by utc_offset_re, and localized StandardNames degrade conservatively) and a case-insensitive placeholder denylist (Local Time, Device Local Time, System Time) closes the hole the suffix anchor left. Tests extended in sources.rs.

Should-fix 8, multi-session finding text (5ce8e2e5). push_esp_link_conflicting now acknowledges the legitimate reimage/retry two-real-sessions case and recommends per-attempt analysis before re-collection.

Fixture note: the two new adversarial scenarios are built inline in tests/intune_windows_autopilot.rs (the file's documented pattern for single-invariant ADR tests) because the issue #362 fixture-directory matrix is pinned as a contract; the corpus-level gap the review flagged (no accessState != available / parseState != parsed input anywhere in CI) is closed by these tests.

Gates at head: cargo test -p cmtraceopen-parser → 2139 passed, 0 failed, 1 ignored; cargo clippy -p cmtraceopen-parser --all-targets -- -D warnings → clean; cargo check --workspace → clean; cargo check --locked -p cmtraceopen-parser --target wasm32-unknown-unknown → clean.

Ultrareview round, event-side closure of the assessability gate (874d322b). The direction-aware gate above covered report SECTIONS only; a capped or raw EVENT carrying a documented failure signal was still silently dropped by the is_assessable filters, so an assessable success path (161 + 153-into-Available + assessable espHandoff section) plus a capped event 172 reduced to a success-family outcome with no finding. Closed as a class: the new recorded_non_assessable_failure_observations iterator gates on AutopilotSignal::is_terminal_failure (171, 172, 807, 809, 815, 908 -- the symmetry rule being that any signal strong enough to fail the enrollment when readable is strong enough to block its success when unreadable; 100 stays out as documented-transient, matching the section iterator's NotFound/Retrying exclusion), and the success branch of reduce_outcome short-circuits to InsufficientEvidence over either record shape. push_non_assessable_failure_recorded widens to cite both shapes, so the recorded failure is never silent. TDD: a_recorded_failure_on_a_non_assessable_event_blocks_success observed red first (reduced to HandoffReachedEspEvidenceMissing, no finding), then green; symmetric control an_assessable_failed_event_still_produces_the_terminal_failure pins that an assessable 172 still yields the terminal ProfileApplicationFailure.

Gates at head: cargo test -p cmtraceopen-parser -> 2141 passed, 0 failed, 1 ignored; cargo clippy -p cmtraceopen-parser --all-targets -- -D warnings -> clean; cargo check --workspace -> clean; cargo check -p cmtraceopen-parser --target wasm32-unknown-unknown -> clean.

Hermes review fixes (3d2ee13d)

The Hermes charter review posted three blocking P1 findings against exact head ed8b50b9. Each is now closed, TDD'd RED-first with the review's exact inputs.

P1 #1, ADR-001 finding-side assessability (rules.rs). signal_evidence filtered only by signal, so a capped/malformed event 171 (TpmIdentityFailed) could still promote autopilot-identity-registration-mismatch to a Blocker/High finding even though reduce_outcome correctly withheld the terminal outcome. signal_evidence now requires is_assessable, and the check is hoisted to one AutopilotObservation::is_assessable method shared by the reducer's free fn and the finding-side helpers so the boundary cannot drift. The recorded failure stays visible via the existing low-confidence autopilot-non-assessable-failure-recorded. TDD: a_non_assessable_tpm_failure_cannot_emit_a_high_confidence_blocker (red first); regression control an_assessable_tpm_failure_still_emits_the_identity_mismatch_blocker.

P1 #2, ADR-004 free-text redaction (redaction.rs). opaque_blob_re was \b[A-Za-z0-9+/=]{40,}\b. The Base64 alphabet includes =, +, /, none of which is a word character, so the trailing \b could not close the match on a padded or punctuation-terminated hardware hash and left its tail exposed (a 40-char value ending in //+ matched nothing at all). The pattern drops both anchors; the greedy, leftmost, >=40 match now consumes the whole contiguous Base64 run in full while the >=40 bound still excludes the 36-char GUID (dashes break it into <=12-char runs) and the short HRESULT, and the [blob:…] token cannot re-match (idempotent). GUID/HRESULT preservation tests still pass. TDD: a_base64_blob_ending_in_punctuation_is_masked_in_full (unit, red first) plus a_base64_hash_in_an_observation_message_never_survives_the_export pinning the whole exported projection for =, ==, +, /.

P1 #3, ADR-003 profile retry linkage (reducer.rs). reduce_profile let a later success (161/153) silently overwrite an earlier explicit negative (NoAssignedProfile 815 / AssignedProfileMissing 809) with no retry linkage, and the result depended on vector order. Positive evidence and explicit negatives are now tracked apart and reconciled from sets (never vector order): a negative is erased only when it shares an activityId retry-linkage key (the module's session/correlation key, matched case-insensitively) with an Available-raising success. Without linkage the negative stands (NoProfileCandidate, not Completed); with linkage the success completes. TDD: an_unlinked_success_cannot_erase_an_earlier_no_profile_negative (red first), linkage-permitted control a_retry_linked_success_completes_over_an_earlier_negative, and input-order-permutation assertion the_profile_linkage_verdict_is_invariant_under_input_order.

Retry-linkage key used: AutopilotObservation.activity_id (and the report section's activityId value), lowercased -- the same key the ESP correlation path already treats as a session identity.

Gates at head: cargo test -p cmtraceopen-parser -> 2150 passed, 0 failed, 1 ignored; cargo clippy -p cmtraceopen-parser --all-targets -- -D warnings -> clean; cargo check --workspace -> clean; cargo check -p cmtraceopen-parser --target wasm32-unknown-unknown -> clean.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes
    • Improved Windows Autopilot outcome detection, including profile application success, failures, ambiguous session links, and conflicting evidence.
    • Improved handling of incomplete or non-assessable records with clearer warnings and evidence re-collection guidance.
    • Standardized redaction for consistent masking regardless of capitalization or surrounding whitespace.
    • Improved timezone and schema validation, including clearer handling of unsupported values.
    • Replaced unstable parser-specific error details with consistent, user-facing messages.
  • Tests
    • Expanded coverage for malformed reports, schema handling, session linkage, evidence requests, redaction, and HRESULT parsing.

adamgell and others added 5 commits August 8, 2026 09:49
ADR-001: non-assessable evidence cannot produce a terminal conclusion.
reduce_profile iterated observations raw, so a capped or unparsed success
record could set retrieved/applied and, with an observed ESP handoff,
prove Completed while the matching failure branch was filtered. The
identity evidence loop had the same bypass and inflated the phase to
IdentityObserved from an unreadable record.

Closing the class, not the instance: sections_of now gates report
sections on their own declared context (covering identity, profile,
handoff, and outcome section paths), and the conflict-mismatch loop,
correlation-key extraction, linkage evidence attribution, and the
time-overlap probe are gated the same way. time_basis stays deliberately
unfiltered because there the unfiltered scan is the conservative
direction; documented at is_assessable.

Also per ADR-003: reduce_outcome now matches a Conflicting ESP linkage
explicitly and returns ContradictoryEvidence. The multi-session match
path records no AutopilotConflict, so the empty-conflicts gate above it
let an ambiguous session identity fall through to Completed.

Refs #362, PR #450.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Two silent-outcome gaps in the findings rules:

- push_unknown_schema gated its capture branch on windows_build alone,
  so a capture declaring only an unvalidated autopilotSchemaVersion
  reduced to UnknownSchema with every terminal rule suppressed and no
  finding saying why. The gate now covers both declared values and the
  summary names whichever failed validation.

- A Conflicting ESP linkage reached through distinct keys matching
  distinct sessions records no AutopilotConflict, so no rule explained
  it. push_esp_link_conflicting covers exactly that path; the
  single-key-many-sessions path stays with push_contradictory_evidence.

ADR-001 (withheld semantics must still be explained) and ADR-003
(conservative representation of unresolved contradictions).

Refs #362, PR #450.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…mezone

windows_zone_re accepted any alphabetic run of three or more characters,
so placeholders like 'unknown', 'not recorded', and 'Unavailable'
classified as Declared. That upgraded time_basis to Utc, raised
reduce_confidence to High, and allowed the TimeOnlyCandidate ESP join
the module contract refuses when the timezone is unrecognizable
(ADR-002: timestamp proximity alone never creates strong correlation).

Every Windows time zone identifier ends in 'Time', so the shape check
anchors on that suffix; UTC and offset forms were already covered by
utc_offset_re.

Also makes detect_document's malformed detail a stable reducer-authored
sentence: serde_json does not guarantee its error Display output across
releases, and the detail flows into golden-asserted finding summaries.

Refs #362, PR #450.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The module contract promises every whole-value mask is computed over the
trimmed, lowercased value, but four sites called stable_token on the raw
value: the ESP matched keys, sensitive named-data values, and both
conflict-value shapes. A sensitive value arriving in a different casing
or with surrounding space therefore masked to a different token than the
identity field it should visibly equal, destroying the cross-field
correlation the projection exists to preserve (ADR-004: same-scope
redaction preserves intended equality). All four sites now go through
mask_value, which also owns the is_token idempotency check.

Also corrects the opaque-blob doc comment (the regex bound is 40, not
32) and pins the deliberate HRESULT canonicalization in normalize.rs:
a 64-bit sign-extended token derives its canonical 32-bit hex while the
raw token survives verbatim, and a genuinely 64-bit value gains no
fabricated 32-bit form.

Refs #362, PR #450.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…utopilot

New inline scenarios pin the hardened invariants: non-assessable success
and identity records cannot prove progress or raise the phase (ADR-001),
a Conflicting ESP linkage reduces to ContradictoryEvidence with an
explaining finding (ADR-003), a schema-version-only unknown capture is
explained, and the native-event input path derives its artifact from the
event's own provenance.

Fixture corrections, each documented in its expected.json assertions:

- malformed-report-section now carries a correctly tagged report
  document with a mangled sections payload, so it exercises the
  report-payload contract instead of generic non-JSON rejection, and its
  golden no longer pins serde_json's unstable error text.
- unknown-windows-schema-version's summary now names the unvalidated
  schema version alongside the build.
- completed-without-esp-bundle's evidence comment no longer claims a
  happy path, and the user-driven manifest describes local-phase
  completion only; post-handoff state belongs to ESP.

update_findings_golden is now #[ignore]d so it never rewrites goldens
beside the tests reading the same files, and write_json goes through a
temp file plus rename so a reader can never observe a truncated golden.

Refs #362, PR #450.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Aug 8, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Autopilot parsing now uses assessability-aware evidence reduction, stable parsing details, normalized redaction, conservative ESP linkage, schema validation findings, and expanded integration coverage. The changes also update fixture metadata and CodeRabbit review settings.

Changes

Autopilot assessment hardening

Layer / File(s) Summary
Input normalization and stable parsing
crates/cmtraceopen-parser/src/intune/enrollment/windows/autopilot/{normalize,redaction,sources,models}.rs
Normalizes HRESULTs and sensitive values. Stabilizes malformed-payload details. Validates Windows timezones and exposes AutopilotObservation::is_assessable.
Assessability-aware reduction
crates/cmtraceopen-parser/src/intune/enrollment/windows/autopilot/reducer.rs
Restricts semantic evidence to assessable records. Reconciles profile retries by activity ID. Separates ESP linkage detection from link proof. Blocks completion on explicit inaccessible failures or mismatches.
ESP linkage and findings
crates/cmtraceopen-parser/src/intune/enrollment/windows/autopilot/rules.rs, crates/cmtraceopen-parser/tests/fixtures/intune/enrollment/windows/autopilot/...
Reports schema gaps, conflicting ESP sessions, and non-assessable recorded failures. Updates expected findings for malformed reports and profile transitions.
Regression fixtures and integration coverage
crates/cmtraceopen-parser/tests/intune_windows_autopilot.rs, crates/cmtraceopen-parser/tests/fixtures/intune/enrollment/windows/autopilot/...
Adds coverage for assessability, identity normalization, linkage, retries, native events, schema handling, redaction, and safe golden-file replacement.
Review workflow configuration
.coderabbit.yaml
Enables request-change workflows, automatic labels, and draft-pull-request reviews.

Estimated code review effort: 5 (Critical) | ~90+ minutes

Sequence Diagram(s)

sequenceDiagram
  participant AutopilotSources
  participant AutopilotReducer
  participant ESPLinkage
  participant FindingsPipeline
  AutopilotSources->>AutopilotReducer: parsed observations and report sections
  AutopilotReducer->>ESPLinkage: assessable and non-assessable correlation keys
  ESPLinkage->>AutopilotReducer: proven, time-only, or conflicting linkage
  AutopilotReducer->>FindingsPipeline: reduced evidence and conservative outcome
  FindingsPipeline->>FindingsPipeline: emit schema, linkage, and non-assessable findings
Loading

Possibly related PRs

  • adamgell/cmtraceopen#450: Directly shares the Autopilot normalization, redaction, reduction, rules, sources, and integration-test paths.
  • adamgell/cmtraceopen#519: Defines reducer contracts and invariants extended by these assessability and linkage changes.
  • adamgell/cmtraceopen#532: Applies similar conservative evidence and correlation handling to a different Intune reducer.

Suggested labels: test

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title follows Conventional Commits and identifies the intune Autopilot changes addressed by the pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch autopilot/framework-hardening

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added bug Something isn't working enhancement New feature or request enrollment Enrollment related intune Microsoft Intune related parser Log parser related labels Aug 8, 2026
@adamgell

adamgell commented Aug 8, 2026

Copy link
Copy Markdown
Owner Author

Consolidated multi-agent review (triage verification + gate-regression analysis, findings adversarially verified)

Verdict: the triage is substantially honest and the two headline fixes are real — but the new assessability gating overcorrects in two places that must be fixed before merge, because they can now suppress failure evidence.

Triage verification

  • All 13 "fixed" findings located in the diff; 11 verified-fixed with pinning tests, including the two majors: the reduce_profile gate (pinned by non_assessable_success_records_cannot_prove_profile_progress) and the Conflicting ESP arm (pinned, and proven to reach the second Conflicting path).
  • All 4 rejections survive scrutiny: 153→ProfileState_Available (172 is the documented failure sibling; a 172 alongside 153 cannot produce Completed), serde Option default (the module doctest omits eventVersion and passes), 64-bit HRESULT sign-extension (test pins raw preservation and the no-fabricated-hex negative case), #[non_exhaustive] (family-wide scoping call; only one use in the whole crate).
  • The NormalizedWindowsEvent deferral is factually clean — no shared-model file touched.

Must-fix before merge (confirmed against the branch)

  1. sections_of gating drops report sections that carry the failure — the reducer can now report Completed/High where it previously reported a terminal failure. reducer.rs:628-637; consumed by the ProfileRetrieval/ProfileApplication Failed probes in reduce_outcome (verified at reducer.rs:1385-1395). A cleanly-parsed report whose profileApplication section says outcome: failed but is declared capped/raw becomes invisible; assessable sibling evidence then completes the enrollment at High confidence (coverage is per-source only, so nothing degrades). Root cause: the gate conflates "cannot support a negative/absence conclusion" with "carries no signal" — Capped means absence proves nothing, not presence proves nothing. The gate must be direction-aware: block success/absence inferences from non-assessable records, but let an explicitly recorded failure block success (or at minimum force a coverage entry + confidence penalty + finding).
  2. autopilot_keys gating can collapse ConflictingLinkedCompleted, defeating this PR's own new arm. reducer.rs:1115-1117 (verified): dropping correlation keys carried only on non-assessable observations can shrink matched_sessions from 2 to 1. Session identity keys are the one input where more conservatism requires keeping the evidence.
  3. The exact redaction site the original finding cited (matched_keys, redaction.rs:208) is the one site the new normalization test never exercises, and no mixed-case-same-key fixture exists. (Behaviorally it is currently a no-op because autopilot_keys lowercases first — pin that with a test rather than leaving it to coincidence.)

Should-fix

  1. mask_value lowercases before hashing while distinct_values is case-sensitive: a case-only identifier conflict now exports "2 distinct values" followed by two identical tokens — a self-contradictory export (ADR-004: redaction must not change conclusions within one analysis). Either make distinct_values case-insensitive (probably the real bug) or preserve distinctness in the tokens.
  2. Triage bookkeeping: the opaque_blob_re doc-comment finding was fixed but omitted from the table (row 20 is self-authored, so "all 20" is 19+1); the contested matching-autopilot-and-esp-session golden lacks the 153/172 reasoning in its assertions array, which is the standard this PR sets for every other contested golden.
  3. has_time_overlap gating silently removes the shared-identifier next_evidence_request when a capped record narrows the AP window (reducer.rs:1280); the outcome is unchanged but the evidence-seeking guidance disappears.
  4. windows_zone_re's Time anchor: conservative when it rejects (degrades to Unreliable, so no correctness bug), but the comment's claim that every Windows zone id ends in Time is false (UTC, UTC+12, ...; localized StandardNames), and the anchor doesn't close the placeholder hole it targets (Local Time, System Time still pass). Fix the comment; consider a placeholder denylist.
  5. Legit multi-session devices (reimage/retry) now get ContradictoryEvidence with remediation text ("re-collect in one pass") that is wrong for that case — the direction is ADR-003-correct, but the finding should acknowledge the two-real-sessions possibility.
  6. No fixture exercises any gated input: no section/observation in the corpus has accessState != available or parseState != parsed, which is exactly why 1-2 are invisible to CI. Add fixtures for capped-failed-section and capped-key-carrying-observation.

Clean checks (verified)

time_basis left ungated is correct (can only push toward Unreliable); coverage denominators aren't shrunk by the gates directly; escalate stays pessimistic; the identity/linkage evidence gates only lower phase; redaction stays idempotent; push_esp_link_conflicting can't double-report with push_contradictory_evidence; the golden-regenerator race fix is correct on both halves.

Review produced by 2 parallel read-only reviewers (triage verification incl. all 20 PR #450 threads; gate-regression/inverse analysis) with load-bearing findings re-verified against the branch. Refs #362, PR #450, ADR-001..004.

adamgell and others added 5 commits August 8, 2026 10:42
… Autopilot success

The assessability gate in sections_of hid every non-assessable report
section from every consumer, including the Failed probes in
reduce_outcome. A capped or unparsed profileApplication section whose
outcome was failed became invisible, and sibling assessable evidence
could then complete the enrollment at high confidence over a failure
that was on record.

The gate is now direction-aware (ADR-001 cuts both ways): sections_of
still admits only assessable sections, so nothing non-assessable can
prove progress or a terminal cause, and the new
recorded_non_assessable_failure_sections iterator carries an explicitly
recorded Failed/Mismatch outcome to the success branch of
reduce_outcome, which then returns InsufficientEvidence instead of
Completed/HandoffReachedEspEvidenceMissing. The recorded failure is
never silent: the new autopilot-non-assessable-failure-recorded finding
(low confidence, warning) cites the section and asks for a readable
re-collection.

NotFound and Retrying stay gated in both directions on purpose: those
are absence or transient statements, and absence in a partial capture
proves nothing.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…tection

autopilot_keys gated every key on assessability, so a key carried only
by a capped observation vanished entirely. When that key was the one
binding a second ESP session, matched_sessions shrank from two to one
and Conflicting collapsed into Linked into Completed: a non-assessable
record silently upgrading the conclusion.

Keys are the one input where more evidence is more conservative: every
additional key can only widen the entangled-session set. So the key set
is now split by AutopilotKeyGate. Proving keys (assessable only) are
still the only ones that can produce Linked and the Completed outcome
behind it; Detecting keys (all observations) feed the multi-session
conflict check, which runs before any positive linkage and returns
Conflicting naming every detected session. A linkage whose only key
rides a non-assessable observation stays NotObserved/TimeOnlyCandidate,
pinned by its own test.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…n its own

The masking loop at the matched_keys site of the redacted export was
behaviorally a no-op because autopilot_keys lowercases every key value
first, and nothing tested either half of that coincidence. Both halves
are now a contract: the reducer hands the projection lowercase values,
and the projection masks a mixed-case value of the same key to the same
token even though the reducer never produces one.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Four coordinated corrections from the PR #531 consolidated review:

- distinct_values now groups case-insensitively with a deterministic
  representative casing (lexicographically smallest sighting). Serials
  and GUIDs are case-insensitive identities, and the redacted export
  masks the trimmed lowercased value, so a case-only difference exported
  as '2 distinct values' over two identical tokens -- a conclusion that
  changed under redaction (ADR-004). A case-only difference is no longer
  a conflict; genuinely distinct values still are, both pinned.

- The shared-identifier next_evidence_request now survives the time
  gate: it is keyed on 'ESP facts supplied but not explicitly linked'
  (NotObserved or TimeOnlyCandidate) instead of on TimeOnlyCandidate
  alone, so narrowing the assessable overlap window can no longer erase
  the one step that advances the diagnosis.

- classify_timezone: the windows_zone_re comment no longer claims every
  Windows zone id ends in 'Time' (UTC/UTC+12 are registry ids too, and
  localized StandardNames only degrade conservatively), and a small
  case-insensitive placeholder denylist (Local Time, Device Local Time,
  System Time) closes the placeholders the suffix anchor let through.

- push_esp_link_conflicting acknowledges the legitimate two-real-
  sessions case: a reimaged or re-enrolled device provisions more than
  once, so the finding now says which situation the reader may be in and
  recommends per-attempt analysis before re-collection.

Also appends the 153/172 application-evidence reasoning to the
matching-autopilot-and-esp-session golden's assertions, the standard the
PR sets for contested goldens.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…utopilot success

The direction-aware assessability gate from the previous round covered
report SECTIONS only. A capped or raw EVENT carrying a documented
failure signal was still silently dropped by the is_assessable filters,
so a bundle with an assessable success path (161 + 153-into-Available +
an assessable espHandoff section) plus a capped event 172 reduced to a
success-family outcome with no finding naming the failure. The class
covers every documented failure signal: 171, 172, 807, 809, 815, 908.

The event side now mirrors the section pattern exactly. The new
recorded_non_assessable_failure_observations iterator selects the
non-assessable observations whose signal is_terminal_failure -- the
symmetry rule being that any record strong enough to fail the enrollment
when readable is strong enough to block its success when unreadable --
and the success branch of reduce_outcome short-circuits to
InsufficientEvidence over either record shape, never a terminal failure
(ADR-001 cuts both ways). ProfilePolicyNotFound (100) stays out for the
same reason the section iterator excludes NotFound/Retrying: documented
transient, not a recorded failure.

push_non_assessable_failure_recorded widens to cite both shapes, so the
recorded failure is never silent; an assessable event 172 still produces
the terminal ProfileApplicationFailure untouched.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@adamgell

adamgell commented Aug 8, 2026

Copy link
Copy Markdown
Owner Author

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented Aug 8, 2026

Copy link
Copy Markdown
✅ Action performed

Full review finished.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
crates/cmtraceopen-parser/src/intune/enrollment/windows/autopilot/reducer.rs (1)

715-745: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Preserve case-sensitive values when grouping distinct values.

Line 738 lowercases every named value. distinct_values also serves hardwareHash, whose Base64 representation is case-sensitive. Two different hashes that differ only by case collapse into one group. single_value can then publish one hash as corroborated identity evidence.

Normalize only keys with a verified case-insensitive identity contract. Keep exact values for hardwareHash and other case-sensitive fields. Add a regression test with two hardware hashes that differ only by case.

🤖 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/enrollment/windows/autopilot/reducer.rs`
around lines 715 - 745, Update distinct_values to use case-insensitive grouping
only for keys with a verified case-insensitive identity contract, while
preserving exact trimmed values for hardwareHash and other case-sensitive
fields. Ensure representative selection and evidence grouping remain
deterministic, and add a regression test covering two hardware hashes that
differ only by case so they remain distinct.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In
`@crates/cmtraceopen-parser/src/intune/enrollment/windows/autopilot/reducer.rs`:
- Around line 1347-1353: Update the Conflicting AutopilotEspLinkage construction
to set matched_keys to an empty vector instead of collecting detected_keys,
while preserving detected_sessions and evidence. Do not alter the public type;
only expose detected keys through a separately versioned schema if that is
already required.

---

Outside diff comments:
In
`@crates/cmtraceopen-parser/src/intune/enrollment/windows/autopilot/reducer.rs`:
- Around line 715-745: Update distinct_values to use case-insensitive grouping
only for keys with a verified case-insensitive identity contract, while
preserving exact trimmed values for hardwareHash and other case-sensitive
fields. Ensure representative selection and evidence grouping remain
deterministic, and add a regression test covering two hardware hashes that
differ only by case so they remain distinct.
🪄 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: 5701bf87-819f-486a-8965-4472234b9de9

📥 Commits

Reviewing files that changed from the base of the PR and between 2e6c03b and 874d322.

📒 Files selected for processing (14)
  • crates/cmtraceopen-parser/src/intune/enrollment/windows/autopilot/normalize.rs
  • crates/cmtraceopen-parser/src/intune/enrollment/windows/autopilot/redaction.rs
  • crates/cmtraceopen-parser/src/intune/enrollment/windows/autopilot/reducer.rs
  • crates/cmtraceopen-parser/src/intune/enrollment/windows/autopilot/rules.rs
  • crates/cmtraceopen-parser/src/intune/enrollment/windows/autopilot/sources.rs
  • crates/cmtraceopen-parser/tests/fixtures/intune/enrollment/windows/autopilot/completed-without-esp-bundle/evidence/autopilot-channel/current/autopilot-events.json
  • crates/cmtraceopen-parser/tests/fixtures/intune/enrollment/windows/autopilot/completed-without-esp-bundle/manifest.json
  • crates/cmtraceopen-parser/tests/fixtures/intune/enrollment/windows/autopilot/malformed-report-section/evidence/mdm-diagnostics-report/current/autopilot-report.json
  • crates/cmtraceopen-parser/tests/fixtures/intune/enrollment/windows/autopilot/malformed-report-section/expected.json
  • crates/cmtraceopen-parser/tests/fixtures/intune/enrollment/windows/autopilot/malformed-report-section/manifest.json
  • crates/cmtraceopen-parser/tests/fixtures/intune/enrollment/windows/autopilot/matching-autopilot-and-esp-session/expected.json
  • crates/cmtraceopen-parser/tests/fixtures/intune/enrollment/windows/autopilot/unknown-windows-schema-version/expected.json
  • crates/cmtraceopen-parser/tests/fixtures/intune/enrollment/windows/autopilot/user-driven-success-through-esp-handoff/manifest.json
  • crates/cmtraceopen-parser/tests/intune_windows_autopilot.rs

…nsitivity

Two CodeRabbit findings on the Conflicting linkage path and value
grouping, both verified against the code before fixing:

- The distinct-keys-to-distinct-sessions Conflicting return exported its
  detecting keys as matched_keys, but AutopilotEspLinkage documents
  matched_keys as empty for every non-Linked state. Detection is not a
  match; the keys stay internal so ambiguity evidence cannot be read as
  proof of a link.

- distinct_values case-folded every named value, including hardwareHash,
  whose Base64 payload is case-sensitive. Two genuinely different hashes
  differing only by case collapsed into one group that single_value then
  published as corroborated identity. Case-insensitive grouping is now
  an explicit allowlist (CASE_INSENSITIVE_VALUE_KEYS) of verified
  case-insensitive Windows identities; every other key compares exactly,
  the conservative direction. All keys exported through detect_conflicts
  remain on the allowlist, preserving the ADR-004 redaction guarantee.

Both fixes landed test-first: the new regression tests failed on the
prior behavior for exactly the reported reasons.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@adamgell

adamgell commented Aug 9, 2026

Copy link
Copy Markdown
Owner Author

@coderabbitai approve

@coderabbitai

coderabbitai Bot commented Aug 9, 2026

Copy link
Copy Markdown
✅ Action performed

Comments resolved. Approval is disabled; enable reviews.request_changes_workflow to allow explicit top-level @coderabbitai resolve or @coderabbitai approve commands.

This branch forked before main's 96b841c enabled
reviews.request_changes_workflow, and CodeRabbit resolves the config
from the PR branch: the @coderabbitai approve command on PR #531
reported "Approval skipped: request-changes workflow disabled". Copy
main's .coderabbit.yaml verbatim so the file carries zero net diff
against main and the formal approval node can be produced.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@coderabbitai coderabbitai Bot added the test Testing related label Aug 9, 2026
@adamgell

adamgell commented Aug 9, 2026

Copy link
Copy Markdown
Owner Author

@coderabbitai approve

@coderabbitai

coderabbitai Bot commented Aug 9, 2026

Copy link
Copy Markdown
✅ Action performed

Comments resolved and changes approved.

@adamgell

adamgell commented Aug 9, 2026

Copy link
Copy Markdown
Owner Author

Hermes charter review

Verdict

Changes requested — 3 blocking semantic findings remain. This is a charter review of PR #531 at exact head ed8b50b9b745abb97e7440869a092212ef546c2c, limited to the Autopilot module diff and its changed Autopilot tests. No files were changed and no threads were resolved.

Findings (ranked)

P1 — Non-assessable TPM failure is still emitted as a high-confidence blocker

Location: crates/cmtraceopen-parser/src/intune/enrollment/windows/autopilot/rules.rs:232-243, 857-866

push_identity_mismatch obtains tpm_failures through signal_evidence, but signal_evidence filters only by signal and does not require is_assessable. Therefore a capped, denied, or malformed event 171 (TpmIdentityFailed) can produce autopilot-identity-registration-mismatch with Blocker severity and High confidence, even though reduce_outcome correctly withholds the terminal outcome for that same record.

Concrete failing input: an event 171 with accessState: capped or parseState: malformed, and no assessable identity-mismatch evidence. The snapshot can contain a high-confidence terminal blocker sourced solely from non-assessable evidence. That violates ADR-001: non-assessable evidence cannot produce a terminal conclusion. The event-side success-blocking gate added in this round does not close this finding-side path.

Disposition: Open / blocking. The finding-side evidence helper needs the same assessability boundary, with a regression test proving that the non-assessable event cannot produce a high-confidence terminal finding. The assessable event-171 behavior should remain covered separately.

P1 — Base64 hardware hashes can remain exposed by free-text redaction

Location: crates/cmtraceopen-parser/src/intune/enrollment/windows/autopilot/redaction.rs:116-121

opaque_blob_re is \\b[A-Za-z0-9+/=]{40,}\\b. The allowed Base64 alphabet includes non-word terminal characters (=, +, /), but the trailing \\b requires a word/non-word boundary. As a result, common padded or slash/plus-terminated Base64 hardware hashes are not fully matched.

Concrete cases: a 40-character value ending in / or + has no match; a padded value ending in == can leave the padding unmasked after a partial match. If such a hash occurs in an observation message, coverage detail, or finding text, raw identity material can survive the exported projection. The existing test only uses "A".repeat(64) and does not exercise Base64 padding or punctuation endings.

Disposition: Open / blocking under the redaction-scope contract. Add regression cases for values ending in =, ==, +, and /, then verify that the complete value is removed from exported text without weakening the existing GUID/HRESULT preservation behavior.

P1 — Unlinked failure-to-success observations can be silently resolved as success

Location: crates/cmtraceopen-parser/src/intune/enrollment/windows/autopilot/reducer.rs:886-941, 982-1004

reduce_profile processes assessable observations but does not retain contradiction or retry/session linkage between explicit negative profile signals (NoAssignedProfile / AssignedProfileMissing, events 815/809) and later success signals (161/153). The direct negative assignment can subsequently be overwritten by a successful retrieval/state transition; raise_candidate then permits Available, and a matching handoff can reach Completed.

Concrete failing input: assessable event 815 (no profile assigned), followed by assessable 161 and 153 (ProfileState_Available), plus an assessable ESP handoff and explicit matching session. The reducer has no transaction/session key establishing that the later success is the same retry rather than a separate attempt. Under ADR-003, retry success may replace failure only when retry linkage is explicit; otherwise the result must remain conservative/ambiguous. There is no regression test for this cross-attempt permutation or for its input-order variants.

Disposition: Open / blocking. Define the workload-local retry/session rule and encode it in adversarial tests before allowing the success transition to erase the earlier explicit negative. Do not infer chronology from vector order.

Contract layer

  • Evidence strength / confidence: Fail for P1 Fix cfg-dependent Clippy unused_mut failure in known log source construction #1. The reducer outcome gate is direction-aware for sections and documented failure events, but push_identity_mismatch still promotes a non-assessable event to a high-confidence terminal finding.
  • Identity / correlation: Fail for P1 Add entry count, position indicator, and severity totals to status bar #3. Explicit ESP key correlation is conservatively separated into proving versus detecting, but local profile failure-to-success transitions lack an explicit attempt boundary.
  • Chronology / terminal precedence: Fail for P1 Add entry count, position indicator, and severity totals to status bar #3. The current profile candidate ranking/overwrite path does not establish retry linkage before treating later success as authoritative.
  • Coverage honesty: Partial. The new section/event non-assessable failure tests, conflict-session tests, schema-only test, and native-event provenance test cover the named prior-round gaps. They do not cover non-assessable failure findings or unlinked 809/815-to-161/153 transitions. Also, push_coverage_gaps only generically reports Missing, PermissionDenied, Capped, and Skipped statuses; ParseFailed/Unsupported are handled only where a specialized document path creates a finding, leaving a potential source-level coverage gap without a generic coverage finding. This is a coverage observation, not a separate blocking finding here.
  • Redaction scope: Fail for P1 Feature: dsregcmd /status Analyzer #2. Whole-value masking normalization and the matched_keys contract are improved, and the hardwareHash case-sensitive allowlist is correct for structured values, but free-text opaque-blob matching does not safely cover the declared Base64 alphabet at value boundaries.
  • Parser purity / framework discipline: No issue found. The changed code remains in the pure parser crate and the branch follows the workload-specific reducer boundary rather than introducing a universal state machine.

Adversarial layer

The following attacks were checked against the exact-head source: non-assessable success and recorded-failure section/event paths; capped correlation keys widening ESP conflict detection without proving linkage; ambiguous multi-session linkage and matched_keys leakage; case-only identity differences; case-sensitive hardwareHash; malformed/unsupported schema paths; unreliable timezone placeholders; native-event provenance; and stable/idempotent structured redaction. The three findings above survive verification. The missing adversarial cases are specifically non-assessable event-171 finding emission, Base64 boundary/padding redaction, and unlinked failure-to-success attempt transitions.

Mechanical layer

  • git diff --check origin/main...origin/autopilot/framework-hardening is clean for the reviewed Autopilot paths.
  • The exact-head GitHub CI checks are all passing, including Rust, TypeScript, E2E, platform builds, MSRV, CodeQL, and ESP Diagnostics (Windows).
  • No native/lab validation claim is made by this review beyond the observed CI check.
  • No repository files were modified during this review.

Gates (observed at head)

Gate State Evidence / disposition
CI PASS gh pr checks 531: all listed checks pass at ed8b50b9; includes Check & Test (Rust), platform builds, TypeScript, E2E, MSRV, CodeQL, and ESP Diagnostics (Windows).
CodeRabbit approved_at_head PASS CodeRabbit formal review is APPROVED at exact head ed8b50b9; the CodeRabbit status check is also passing. The prior matched_keys and hardwareHash comments are verified as addressed.
Contract conformance FAIL ADR-001 finding-side assessability, ADR-003 retry/terminal precedence, and ADR-004 complete free-text redaction are not yet demonstrated by the exact-head implementation/tests.
Hermes charter review POSTING This is the required single top-level Hermes charter comment. Its blocking findings remain unresolved.
Merge readiness NOT READY CodeRabbit and CI are green, but the contract gate and Hermes blocking-findings gate are not green. Merging remains Adam's action.

Dispositions of prior-round feedback

  • Accepted as fixed: direction-aware non-assessable section and event success gates; assessability coverage across reduction consumers; capped-key conflict detection; explicit conflicting-linkage outcome; empty matched_keys for non-Linked states; case-insensitive grouping only for the verified identity allowlist; case-sensitive hardwareHash structured grouping; schema-only explanation; stable parser-authored parse details; timezone placeholder handling; evidence-request survival; native-event provenance; safe ignored golden regeneration.
  • Rejected/deferred as reasonable: the prior review's eventVersion serde-default concern, family-wide #[non_exhaustive] request, sign-extended HRESULT canonicalization concern, and the shared normalized-model release-management concern. Those dispositions are consistent with the code and repository conventions and are not reopened here.
  • Not accepted as closed: the three findings above are distinct paths not covered by the prior fix round or its regression matrix.

Coverage statement

This review read the charter and routing indexes first, then the repository specialist context, ADR-001..004, reducer checklist, reviewer charters, the PR body/history, the exact origin/main...origin/autopilot/framework-hardening diff, exact-head branch files via git show, changed tests, GitHub CI, and CodeRabbit review state. It covered contract, adversarial, and mechanical layers for the Autopilot module. It did not claim Windows lab acceptance, did not inspect unrelated modules beyond the contract context, and did not modify code, resolve threads, or merge.

Final state: CI PASS; CodeRabbit approved at head; contract conformance FAIL; Hermes review posted with 3 blocking P1 findings; PR #531 is not merge-ready.

Address the three blocking semantic findings from the Hermes charter review
of PR #531, each TDD'd (RED first with Hermes's exact inputs).

1. ADR-001 finding side: `signal_evidence` (rules.rs) now requires
   `is_assessable`, so a capped/malformed event 171 can no longer promote
   `autopilot-identity-registration-mismatch` to a High/Blocker finding the
   reducer's own outcome gate already withholds. The assessability check is
   hoisted to a single `AutopilotObservation::is_assessable` method shared by
   the reducer's free fn and the finding-side helpers so the boundary cannot
   drift. Assessable-171 still fires (regression pinned).

2. ADR-004 redaction: `opaque_blob_re` drops its `\b` anchors. The Base64
   alphabet includes `=`, `+`, `/` (non-word), so a trailing `\b` could not
   close a match on a padded/punctuation-terminated hardware hash and left its
   tail exposed. The greedy, leftmost, >=40 match now consumes the whole
   contiguous run while still excluding the 36-char GUID and short HRESULT, and
   the `[blob:...]` token cannot re-match (idempotent).

3. ADR-003 chronology: `reduce_profile` no longer lets an unlinked later
   success (161/153) silently erase an earlier explicit negative (815/809).
   Positive evidence and explicit negatives are tracked apart and reconciled
   from sets (never vector order); a negative is erased only when it shares an
   `activityId` retry-linkage key with an Available-raising success. Unlinked
   stays conservative (NoProfileCandidate, not Completed); linked completes.

Gates: cargo test -p cmtraceopen-parser (0 failures), clippy -D warnings,
cargo check --workspace, wasm32 check all clean.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@adamgell

adamgell commented Aug 9, 2026

Copy link
Copy Markdown
Owner Author

@/private/tmp/claude-502/-Users-Adam-Gell-repo-cmtraceopen/95f00e8d-9b26-48f6-ab69-f2216059e3f3/scratchpad/hermes_reply.md

@adamgell

adamgell commented Aug 9, 2026

Copy link
Copy Markdown
Owner Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Aug 9, 2026

Copy link
Copy Markdown
⚠️ Action not completed

Already reviewed.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In
`@crates/cmtraceopen-parser/src/intune/enrollment/windows/autopilot/reducer.rs`:
- Around line 1009-1017: Update the doc comment for section_activity_id to state
that it returns the activityId value verbatim and does not lowercase it; note
that normalization is performed by push_link instead.

In `@crates/cmtraceopen-parser/tests/intune_windows_autopilot.rs`:
- Around line 1886-1894: Strengthen the redacted_export_projection test around
the existing text assertion by also verifying that the distinctive
repeated-character run from the Base64 blob is absent from the serialized
output. Keep the full-blob assertion and add a separate check that would fail if
redaction leaves trailing “=”, “+”, or “/” characters.
🪄 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: f820bf4f-9f05-460a-a6d8-fd63091845a9

📥 Commits

Reviewing files that changed from the base of the PR and between ed8b50b and 3d2ee13.

📒 Files selected for processing (5)
  • crates/cmtraceopen-parser/src/intune/enrollment/windows/autopilot/models.rs
  • crates/cmtraceopen-parser/src/intune/enrollment/windows/autopilot/redaction.rs
  • crates/cmtraceopen-parser/src/intune/enrollment/windows/autopilot/reducer.rs
  • crates/cmtraceopen-parser/src/intune/enrollment/windows/autopilot/rules.rs
  • crates/cmtraceopen-parser/tests/intune_windows_autopilot.rs

Comment thread crates/cmtraceopen-parser/src/intune/enrollment/windows/autopilot/reducer.rs Outdated
Comment thread crates/cmtraceopen-parser/tests/intune_windows_autopilot.rs
@adamgell

adamgell commented Aug 9, 2026

Copy link
Copy Markdown
Owner Author

Hermes charter re-review (head 3d2ee13)

Reviewed PR #531 at exact head 3d2ee13ddde05fed21486df477eb9ad8fe816b57. Branch files were inspected read-only via git show origin/autopilot/framework-hardening:PATH; no checkout, file changes, thread resolution, or merge was performed.

Prior blocking findings

  1. P1 — Non-assessable event 171 could emit a High/Blocker identity-mismatch finding: CLOSED.
    AutopilotObservation::is_assessable() now requires both access_state == Available and parse_state == Parsed. Reducer observation consumers use that boundary, and rules.rs::signal_evidence applies it before supplying evidence to terminal findings. push_non_assessable_failure_recorded explicitly handles the inverse path and emits only the low-confidence recorded-failure coverage finding. The exact-head regression pair confirms that capped event 171 does not produce autopilot-identity-registration-mismatch, while an assessable event 171 still produces the Blocker/High finding. No non-assessable event-171 terminal finding path remains.

  2. P1 — Base64 opaque blobs ending in =, ==, +, or / could remain partially unmasked: CLOSED.
    opaque_blob_re is now [A-Za-z0-9+/=]{40,} with the trailing \\b removed. This consumes the complete contiguous Base64 run, including punctuation-ending padding/suffixes. The >=40 bound remains: GUIDs are separated by dashes into short runs and HRESULTs are much shorter, so GUID/HRESULT diagnostic preservation is not weakened. The replacement token contains characters outside the Base64 alphabet, so a second projection cannot rematch it; idempotence is preserved. The exact-head regression covers all four endings through the exported observation-message path.

  3. P1 — An unlinked later success could erase explicit 815/809 negative evidence: CLOSED.
    reduce_profile now accumulates explicit negatives and positive Available evidence independently, then reconciles them without vector-order chronology. A negative is replaced only when a success shares an explicit normalized activity_id; otherwise the negative wins. Exact-head tests cover unlinked 815 → 161/153, the linked retry case, and input permutation invariance. The unlinked path is conservative: it returns NoProfileCandidate rather than allowing an unrelated success to reach Completed.

Specific implementation scrutiny

(a) Assessability boundary: PASS.

The shared method is defined on AutopilotObservation and is consumed through the reducer's is_assessable wrapper, signal_observations, profile/identity reduction, evidence grouping, correlation proof gates, and the finding-side signal_evidence and push_non_assessable_failure_recorded paths. The scan found no observation semantic consumer that bypasses it. Report sections use the equivalent is_assessable_section gate against the same two context fields; section observations are also covered by the observation-level failure finding. The intentional exception is time_basis, which remains unfiltered because non-assessable timestamps may only conservatively downgrade time quality; filtering them could incorrectly upgrade it. Detecting-only ESP key collection also intentionally retains non-assessable keys to widen conflict detection, while proof/linkage remains assessability-gated. Those are conservative, documented direction-aware exceptions, not boundary drift.

(b) Retry-linkage identity: PASS, with workload-local scope.

activity_id is the right key for this profile-event workload under ADR-003: the normalized Windows event contract describes it as correlating a multi-event operation, and the Autopilot module already treats explicit enrollment/correlation/activity/device identifiers as the only cross-boundary keys. The implementation does not infer chronology from vector order or timestamps. It reads event activityId from the normalized event and report-section activityId from section values, trims at key collection, lowercases for case-insensitive comparison, and requires a shared key on both the explicit negative and an Available success. Missing or mismatched keys do not link attempts. Thus the unlinked path is genuinely conservative, not merely a different winner-selection policy. The branch also retains the separate ESP correlation semantics; the profile retry rule does not use time proximity.

One limitation noted: the new retry regression matrix exercises event activityId linkage directly; the report-section extraction is present and used, but is not independently pinned by a similarly focused section-negative/section-success fixture. That is a coverage suggestion, not a surviving blocker, because the extraction and the conservative missing-key behavior are direct and the existing section path is assessability-gated.

Contract and adversarial verdict

  • Evidence strength / confidence: PASS — non-assessable evidence cannot drive terminal findings/outcomes; recorded failures are surfaced as low-confidence uncertainty.
  • Identity / correlation: PASS — explicit activity linkage is required for retry replacement; ESP proof remains assessability-gated and ambiguity remains conflicting.
  • Chronology / terminal precedence: PASS — no caller-order overwrite; unlinked success cannot erase 815/809.
  • Coverage honesty: PASS for the three re-review findings — exact-head tests cover capped 171, all four opaque-blob endings, unlinked/linked/permuted retry cases, plus assessable controls.
  • Redaction scope: PASS — complete opaque runs are masked, GUID/HRESULT grammar remains preserved, and tokens are idempotent.
  • Parser purity / framework discipline: PASS — changes remain in the pure parser crate.

Gates observed at head

Gate State Evidence
CI PENDING Check & Test (Rust) and ESP Diagnostics (Windows) were still in progress when checked; other listed CI checks were passing.
CodeRabbit approved_at_head FAIL / NOT APPROVED The latest CodeRabbit review is CHANGES_REQUESTED at exact head 3d2ee13d; the CodeRabbit status was not green/approved at the time of review.
Contract conformance PASS for the reviewed blockers All three prior P1 findings are closed against the exact-head implementation and regression tests.
Hermes charter review POSTING This single top-level re-review comment.
Merge readiness NOT READY The three Hermes blocking findings are cleared, but CI is incomplete and CodeRabbit is not approved at this head.

Clear statement: The three blocking findings from the prior Hermes review are genuinely closed at head 3d2ee13d. They are not merge blockers now. PR #531 is nevertheless not merge-ready until the remaining CI checks pass and CodeRabbit is approved at this exact head.

This review covered the charter, routing indexes, reducer ADRs/checklist, exact-head Autopilot implementation and tests, PR gate state, all three prior findings, and the two requested implementation choices. It did not claim native Windows/lab validation.

…b test

CodeRabbit's two trivial findings on the Hermes P1 round, both valid.
The section_activity_id doc claimed it lowercased its return value; it
returns verbatim and push_link owns the lowercasing, so the doc now
says where the normalization lives. The Base64 export test asserted
only that the whole blob was absent, which would have passed while a
masked body left its punctuation tail behind - the exact partial match
the old word-boundary pattern produced - so it now asserts the body
and the dangling tail are gone too.

Verified: cargo test -p cmtraceopen-parser 2150 passed 0 failed
(autopilot suite 47 passed), clippy -D warnings clean.

Refs #362

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@adamgell

adamgell commented Aug 9, 2026

Copy link
Copy Markdown
Owner Author

Hermes confirmation (head ae6e2ac)

Clearance carries to this head.

The delta from 3d2ee13 to ae6e2ac is doc/test-only: the reducer change updates a comment, and the test change strengthens the Base64 export assertion. The added body check catches a partially redacted blob, and the immediate punctuation-tail check catches a dangling =, +, or / after the mask token; the test therefore cannot pass with the tail leak described in the prior P1.

I re-checked the exact-head branch read-only via git show. The three prior Hermes P1 findings remain closed: non-assessable event-171 evidence is excluded from terminal findings, opaque Base64 runs are fully masked, and unlinked profile failure-to-success transitions remain conservative and require explicit activity linkage.

@adamgell
adamgell marked this pull request as ready for review August 9, 2026 14:53
Copilot AI lite review requested due to automatic review settings August 9, 2026 14:53
@adamgell
adamgell merged commit 640396b into main Aug 9, 2026
16 checks passed

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens the Intune Windows Autopilot reducer by applying stricter “Reducer Framework v1” discipline to assessability gating, ESP linkage conflict handling, and redaction stability—then pins the corrected behavior with expanded tests and refreshed fixture expectations.

Changes:

  • Tightens assessability gates (including direction-aware “recorded failure blocks success” behavior) across reducer paths and finding derivation.
  • Improves ESP linkage logic to conservatively detect multi-session conflicts (including via non-assessable key carriers) and to avoid silent upgrades to Completed.
  • Stabilizes redaction and parse-failure messaging (removing dependence on unstable serde_json error display text) and expands regression coverage.

Reviewed changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
crates/cmtraceopen-parser/tests/intune_windows_autopilot.rs Adds targeted inline invariant tests (assessability, linkage conflicts, redaction contracts) and changes golden regeneration behavior.
crates/cmtraceopen-parser/tests/fixtures/intune/enrollment/windows/autopilot/user-driven-success-through-esp-handoff/manifest.json Clarifies scenario description to reflect Autopilot-vs-ESP boundary.
crates/cmtraceopen-parser/tests/fixtures/intune/enrollment/windows/autopilot/unknown-windows-schema-version/expected.json Updates assertions and unknown-schema finding summary to explain withheld semantics more completely.
crates/cmtraceopen-parser/tests/fixtures/intune/enrollment/windows/autopilot/matching-autopilot-and-esp-session/expected.json Documents the reasoning for treating event 153 into ProfileState_Available as application evidence.
crates/cmtraceopen-parser/tests/fixtures/intune/enrollment/windows/autopilot/malformed-report-section/manifest.json Updates metadata (bytesCopied) to match the corrected fixture payload.
crates/cmtraceopen-parser/tests/fixtures/intune/enrollment/windows/autopilot/malformed-report-section/expected.json Replaces unstable serde error text with stable reducer-authored messaging; updates scenario assertions.
crates/cmtraceopen-parser/tests/fixtures/intune/enrollment/windows/autopilot/malformed-report-section/evidence/mdm-diagnostics-report/current/autopilot-report.json Switches from XML to a tagged JSON envelope with a mangled sections payload to exercise the intended contract.
crates/cmtraceopen-parser/tests/fixtures/intune/enrollment/windows/autopilot/completed-without-esp-bundle/manifest.json Updates metadata (bytesCopied) after fixture content adjustment.
crates/cmtraceopen-parser/tests/fixtures/intune/enrollment/windows/autopilot/completed-without-esp-bundle/evidence/autopilot-channel/current/autopilot-events.json Corrects fixture comment to describe the “handoff reached but no ESP evidence captured” scenario.
crates/cmtraceopen-parser/src/intune/enrollment/windows/autopilot/sources.rs Stabilizes malformed-document detail and tightens timezone classification (Windows zone shape + placeholder denylist).
crates/cmtraceopen-parser/src/intune/enrollment/windows/autopilot/rules.rs Adds missing findings (ESP linkage conflicting; non-assessable failure recorded), and fixes unknown-schema explanation coverage.
crates/cmtraceopen-parser/src/intune/enrollment/windows/autopilot/reducer.rs Implements direction-aware assessability gating, improves linkage conflict detection/proof separation, stabilizes payload parse errors, and makes profile retry linkage order-invariant.
crates/cmtraceopen-parser/src/intune/enrollment/windows/autopilot/redaction.rs Fixes Base64 blob matching edge cases, and enforces consistent whole-value normalization/masking at all sites.
crates/cmtraceopen-parser/src/intune/enrollment/windows/autopilot/normalize.rs Adds regression test covering sign-extended HRESULT normalization semantics.
crates/cmtraceopen-parser/src/intune/enrollment/windows/autopilot/models.rs Centralizes assessability logic on AutopilotObservation::is_assessable to prevent reducer/finding drift.
.coderabbit.yaml Changes CodeRabbit review automation behavior (request-changes workflow, auto-labels, draft reviews).

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines +196 to 200
Err(_) => {
return AutopilotDocumentDetection::Malformed {
detail: format!("content is not a JSON object: {error}"),
detail: "content is not a JSON object".to_owned(),
}
}
Comment on lines +1943 to +1947
let temporary = path.with_extension("json.tmp");
std::fs::write(&temporary, text)
.unwrap_or_else(|error| panic!("{} is writable: {error}", temporary.display()));
std::fs::rename(&temporary, path)
.unwrap_or_else(|error| panic!("{} is replaceable: {error}", path.display()));
Comment thread .coderabbit.yaml
Comment on lines 14 to 18
profile: assertive
# Keep reviews advisory: CI gates (cargo clippy, cargo test, tsc) decide mergeability.
request_changes_workflow: false
request_changes_workflow: true
high_level_summary: true
changed_files_summary: true
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working enhancement New feature or request enrollment Enrollment related intune Microsoft Intune related parser Log parser related test Testing related

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants