fix(sccm): preserve symmetric sparse site-core coverage - #439
Merged
adamgell merged 19 commits intoAug 2, 2026
Merged
Conversation
github-actions
Bot
added
bug
📝 WalkthroughWalkthroughAdds a public SCCM site-core reducer. It validates intake evidence, parses profile records, reduces transactions into workflow states, emits findings and artifact requests, and adds fixtures and integration tests. ChangesSCCM site-core analysis
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Comment |
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
🟢 Ready to approve
The changes align fixtures and contract tests with the updated site-core analysis behavior and add targeted regression coverage for unsafe/malformed artifact IDs without introducing evident correctness issues.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
Updates the SCCM server site-core analysis and its corpus/tests to preserve sparse coverage diagnostics symmetrically when peer evidence references unsafe artifact identifiers, ensuring findings aren’t silently dropped and rejected IDs are projected into deterministic canonical identities.
Changes:
- Adjusts site-core fixture contract expectations around
fragmentCompleteplacement and canonical rotation/request candidate representations. - Adds an extensive
sccm_server_site_coretest suite, including cases for oversized and illegal (e.g., slash-containing) artifact IDs and deterministic rejected-artifact identities. - Refreshes multiple site-core corpus fixtures (manifests, expected outputs, and synthetic evidence) to match the updated schema/output shape and canonical rotation handling.
File summaries
| File | Description |
|---|---|
| crates/cmtraceopen-parser/src/sccm/server/windows/mod.rs | Exposes the site-core module via the Windows SCCM server surface. |
| crates/cmtraceopen-parser/tests/sccm_site_core_fixture_contract.rs | Updates contract assertions for fragment completeness and rotation/request candidate shape. |
| crates/cmtraceopen-parser/tests/sccm_server_site_core.rs | Adds comprehensive reducer/intake-driven site-core tests, including malformed/unsafe ID scenarios. |
| crates/cmtraceopen-parser/tests/fixtures/sccm/server/site_core/status-processing-failure/manifest.json | Updates fixture manifest schema/fields for the scenario. |
| crates/cmtraceopen-parser/tests/fixtures/sccm/server/site_core/status-processing-failure/expected.json | Updates expected analysis output to the current schema/result/finding shapes. |
| crates/cmtraceopen-parser/tests/fixtures/sccm/server/site_core/status-processing-failure/evidence/sccm/server/site-server/server-status/current/statmgr.log | Adds synthetic status evidence for the scenario. |
| crates/cmtraceopen-parser/tests/fixtures/sccm/server/site_core/status-processing-failure/evidence/sccm/server/site-server/server-sitecomp/current/sitecomp.log | Adds synthetic sitecomp evidence for the scenario. |
| crates/cmtraceopen-parser/tests/fixtures/sccm/server/site_core/rotation-boundary/manifest.json | Updates rotation-boundary manifest to current intake schema and rotation fields. |
| crates/cmtraceopen-parser/tests/fixtures/sccm/server/site_core/rotation-boundary/expected.json | Updates expected coverage-gap/requests output structure for rotation-boundary behavior. |
| crates/cmtraceopen-parser/tests/fixtures/sccm/server/site_core/rotation-boundary/evidence/sccm/server/site-server/server-sitecomp/lo_/sitecomp.lo_ | Adds synthetic rollover fragment evidence used to validate rotation handling. |
| crates/cmtraceopen-parser/tests/fixtures/sccm/server/site_core/rotation-boundary/evidence/sccm/server/site-server/server-sitecomp/current/sitecomp.log | Adds synthetic current-fragment evidence for the rotation-boundary scenario. |
| crates/cmtraceopen-parser/tests/fixtures/sccm/server/site_core/recovery/manifest.json | Updates recovery manifest to current intake schema/fields. |
| crates/cmtraceopen-parser/tests/fixtures/sccm/server/site_core/recovery/expected.json | Updates expected recovery output to current schema/result/finding shapes. |
| crates/cmtraceopen-parser/tests/fixtures/sccm/server/site_core/recovery/evidence/sccm/server/site-server/server-status/current/statmgr.log | Adds synthetic status evidence for recovery scenario. |
| crates/cmtraceopen-parser/tests/fixtures/sccm/server/site_core/recovery/evidence/sccm/server/site-server/server-sitecomp/current/sitecomp.log | Adds synthetic sitecomp evidence for recovery scenario. |
| crates/cmtraceopen-parser/tests/fixtures/sccm/server/site_core/malformed/manifest.json | Updates malformed manifest to current intake schema/fields. |
| crates/cmtraceopen-parser/tests/fixtures/sccm/server/site_core/malformed/expected.json | Updates expected malformed output to current schema, including coverage gap + request shapes. |
| crates/cmtraceopen-parser/tests/fixtures/sccm/server/site_core/malformed/evidence/sccm/server/site-server/server-status/current/statmgr.log | Adds synthetic malformed status evidence to drive parse-failed coverage behavior. |
| crates/cmtraceopen-parser/tests/fixtures/sccm/server/site_core/incomplete/manifest.json | Updates incomplete manifest schema and declared coverage states. |
| crates/cmtraceopen-parser/tests/fixtures/sccm/server/site_core/incomplete/expected.json | Updates expected incomplete output to current schema, including multiple coverage gaps/findings/requests. |
| crates/cmtraceopen-parser/tests/fixtures/sccm/server/site_core/incomplete/evidence/sccm/server/site-server/server-sitecomp/current/sitecomp.log | Adds synthetic incomplete sitecomp evidence for capped/parse-gap behavior. |
| crates/cmtraceopen-parser/tests/fixtures/sccm/server/site_core/inbox-backlog/manifest.json | Updates inbox-backlog manifest to current intake schema/fields. |
| crates/cmtraceopen-parser/tests/fixtures/sccm/server/site_core/inbox-backlog/expected.json | Updates expected inbox-backlog output to current schema, including request candidate pairs. |
| crates/cmtraceopen-parser/tests/fixtures/sccm/server/site_core/inbox-backlog/evidence/sccm/server/site-server/server-sitecomp/current/sitecomp.log | Adds synthetic inbox backlog evidence. |
| crates/cmtraceopen-parser/tests/fixtures/sccm/server/site_core/healthy/manifest.json | Updates healthy manifest to current intake schema/fields. |
| crates/cmtraceopen-parser/tests/fixtures/sccm/server/site_core/healthy/expected.json | Updates expected healthy output to current schema (results/findings/requests fields). |
| crates/cmtraceopen-parser/tests/fixtures/sccm/server/site_core/healthy/evidence/sccm/server/site-server/server-status/current/statmgr.log | Adds synthetic healthy status evidence. |
| crates/cmtraceopen-parser/tests/fixtures/sccm/server/site_core/healthy/evidence/sccm/server/site-server/server-sitecomp/current/sitecomp.log | Adds synthetic healthy sitecomp evidence. |
| crates/cmtraceopen-parser/tests/fixtures/sccm/server/site_core/contradictory/manifest.json | Updates contradictory manifest to current intake schema/fields. |
| crates/cmtraceopen-parser/tests/fixtures/sccm/server/site_core/contradictory/expected.json | Updates expected contradictory output to current schema, including result/finding layout. |
| crates/cmtraceopen-parser/tests/fixtures/sccm/server/site_core/contradictory/evidence/sccm/server/site-server/server-status/current/statmgr.log | Adds synthetic contradictory status evidence. |
| crates/cmtraceopen-parser/tests/fixtures/sccm/server/site_core/contradictory/evidence/sccm/server/site-server/server-sitecomp/current/sitecomp.log | Adds synthetic contradictory sitecomp evidence. |
| crates/cmtraceopen-parser/tests/fixtures/sccm/server/site_core/component-failure/manifest.json | Updates component-failure manifest to current intake schema/fields. |
| crates/cmtraceopen-parser/tests/fixtures/sccm/server/site_core/component-failure/expected.json | Updates expected component-failure output to current schema, including coverage gaps/findings/requests. |
| crates/cmtraceopen-parser/tests/fixtures/sccm/server/site_core/component-failure/evidence/sccm/server/site-server/server-sitecomp/current/sitecomp.log | Adds synthetic terminal component failure evidence. |
Review details
- Files reviewed: 22/36 changed files
- Comments generated: 0
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
18 tasks
![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
🧹 Nitpick comments (4)
crates/cmtraceopen-parser/src/sccm/server/windows/site_core.rs (2)
497-511: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAlign the duplicate-identity scope with
collect_coverage_gaps.
admitted_sourcescounts occurrences over every artifact.collect_coverage_gapscounts occurrences only over site-core artifacts (Line 746). If one site-core artifact and one non-site-core artifact share anartifact_id, this function drops the site-core artifact, butcollect_coverage_gapsdoes not classify it asduplicate-source-identity. The gap then reports the genericsource-contract-rejectedreason. Use the same site-core-only counting in both functions so the reason code stays accurate.♻️ Proposed change
let mut occurrences = BTreeMap::<&str, usize>::new(); for artifact in &intake.artifacts { + if SiteCoreGroup::from_source_id(&artifact.source_id).is_none() { + continue; + } *occurrences .entry(artifact.artifact_id.as_str()) .or_default() += 1; }🤖 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/sccm/server/windows/site_core.rs` around lines 497 - 511, Update the occurrence counting in admitted_sources to use only artifacts that resolve through SiteCoreGroup::from_source_id, matching collect_coverage_gaps. Ensure non-site-core artifacts do not affect duplicate artifact_id detection, while preserving the existing filtering and source-contract behavior for site-core artifacts.
1811-1825: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the byte-offset invariant between
lowercaseandmessage.This function takes match offsets from
lowercaseand slicesmessagewith them. The offsets stay valid only becauseto_ascii_lowercasepreserves byte length and char boundaries. A later change toto_lowercasewould break the invariant and panic on non-ASCII input. Add a short comment that states the requirement.♻️ Proposed change
fn validated_token_value(message: &str, label: &str) -> Option<Option<String>> { + // `to_ascii_lowercase` preserves byte length and char boundaries, so offsets + // found in `lowercase` are valid indices into `message`. Do not use `to_lowercase`. let lowercase = message.to_ascii_lowercase();🤖 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/sccm/server/windows/site_core.rs` around lines 1811 - 1825, Add a short comment in validated_token_value immediately after creating lowercase, documenting that its match offsets may slice message only because ASCII lowercasing preserves byte length and character boundaries; warn that replacing it with Unicode lowercasing would invalidate this invariant.crates/cmtraceopen-parser/tests/fixtures/sccm/server/site_core/incomplete/manifest.json (1)
46-58: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign the third artifact identifiers with its
server-statussource.This artifact uses
sourceId: "server-status"andoriginalBasename: "statesys.log", but itsartifactIdisb-sitecompand itsrotation.lineageIdissitecomp-a. The sitecomp-style names do not describe a status source. Rename them to status-style identifiers so later fixture edits do not attach the wrong source semantics. Note that the artifact ids feedexpected.json, so regenerate the expected output after any rename.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/cmtraceopen-parser/tests/fixtures/sccm/server/site_core/incomplete/manifest.json` around lines 46 - 58, Update the third artifact’s identifiers in the manifest to use status-style names consistent with its server-status source and statesys.log basename: rename both artifactId and rotation.lineageId, then regenerate expected.json so all derived references match.crates/cmtraceopen-parser/tests/sccm_server_site_core.rs (1)
1671-1693: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse
replace_source_artifact_idfor the oversized-identity setup.Lines 1674-1689 repeat the artifact, coverage, and evidence rewrite that
replace_source_artifact_idalready performs. Call the helper first, then apply only the extraevidence_id/entry_idmutation. This keeps one definition of the identity rewrite contract.♻️ Proposed refactor
- assessment - .artifacts - .iter_mut() - .find(|artifact| artifact.source_id == "server-sitecomp") - .expect("sitecomp artifact") - .artifact_id = oversized_id.clone(); - for coverage in &mut assessment.coverage { - for artifact_id in &mut coverage.artifact_ids { - if artifact_id == "sitecomp-current" { - *artifact_id = oversized_id.clone(); - } - } - } + replace_source_artifact_id(&mut assessment, "server-sitecomp", &oversized_id); for evidence in &mut assessment.evidence { - if evidence.reference.artifact_id == "sitecomp-current" { - evidence.reference.artifact_id = oversized_id.clone(); + if evidence.reference.artifact_id == oversized_id { evidence.evidence_id = format!("{oversized_id}:{}", evidence.evidence_id); evidence.reference.entry_id = evidence.evidence_id.clone(); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/cmtraceopen-parser/tests/sccm_server_site_core.rs` around lines 1671 - 1693, Update invalid_finding_inputs_become_explicit_gaps_instead_of_clearing_class to call replace_source_artifact_id for the oversized identity setup instead of manually rewriting artifact, coverage, and evidence references. After the helper call, retain only the additional evidence_id and reference.entry_id mutation required by this test.
🤖 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/sccm/server/windows/site_core.rs`:
- Around line 1668-1689: Update the phase construction in the finding builder
around last_successful_phase so None emits the explicit unconfirmed phase token
rather than SccmSiteCorePhase::ComponentStart; retain serialized_name() for
confirmed phases and update the affected expected.json fixtures to match.
---
Nitpick comments:
In `@crates/cmtraceopen-parser/src/sccm/server/windows/site_core.rs`:
- Around line 497-511: Update the occurrence counting in admitted_sources to use
only artifacts that resolve through SiteCoreGroup::from_source_id, matching
collect_coverage_gaps. Ensure non-site-core artifacts do not affect duplicate
artifact_id detection, while preserving the existing filtering and
source-contract behavior for site-core artifacts.
- Around line 1811-1825: Add a short comment in validated_token_value
immediately after creating lowercase, documenting that its match offsets may
slice message only because ASCII lowercasing preserves byte length and character
boundaries; warn that replacing it with Unicode lowercasing would invalidate
this invariant.
In
`@crates/cmtraceopen-parser/tests/fixtures/sccm/server/site_core/incomplete/manifest.json`:
- Around line 46-58: Update the third artifact’s identifiers in the manifest to
use status-style names consistent with its server-status source and statesys.log
basename: rename both artifactId and rotation.lineageId, then regenerate
expected.json so all derived references match.
In `@crates/cmtraceopen-parser/tests/sccm_server_site_core.rs`:
- Around line 1671-1693: Update
invalid_finding_inputs_become_explicit_gaps_instead_of_clearing_class to call
replace_source_artifact_id for the oversized identity setup instead of manually
rewriting artifact, coverage, and evidence references. After the helper call,
retain only the additional evidence_id and reference.entry_id mutation required
by this test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e300c902-313d-4690-b588-e5c5a52934f7
⛔ Files ignored due to path filters (13)
crates/cmtraceopen-parser/tests/fixtures/sccm/server/site_core/component-failure/evidence/sccm/server/site-server/server-sitecomp/current/sitecomp.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/server/site_core/contradictory/evidence/sccm/server/site-server/server-sitecomp/current/sitecomp.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/server/site_core/contradictory/evidence/sccm/server/site-server/server-status/current/statmgr.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/server/site_core/healthy/evidence/sccm/server/site-server/server-sitecomp/current/sitecomp.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/server/site_core/healthy/evidence/sccm/server/site-server/server-status/current/statmgr.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/server/site_core/inbox-backlog/evidence/sccm/server/site-server/server-sitecomp/current/sitecomp.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/server/site_core/incomplete/evidence/sccm/server/site-server/server-sitecomp/current/sitecomp.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/server/site_core/malformed/evidence/sccm/server/site-server/server-status/current/statmgr.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/server/site_core/recovery/evidence/sccm/server/site-server/server-sitecomp/current/sitecomp.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/server/site_core/recovery/evidence/sccm/server/site-server/server-status/current/statmgr.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/server/site_core/rotation-boundary/evidence/sccm/server/site-server/server-sitecomp/current/sitecomp.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/server/site_core/status-processing-failure/evidence/sccm/server/site-server/server-sitecomp/current/sitecomp.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/server/site_core/status-processing-failure/evidence/sccm/server/site-server/server-status/current/statmgr.logis excluded by!**/*.log
📒 Files selected for processing (23)
crates/cmtraceopen-parser/src/sccm/server/windows/mod.rscrates/cmtraceopen-parser/src/sccm/server/windows/site_core.rscrates/cmtraceopen-parser/tests/fixtures/sccm/server/site_core/component-failure/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/server/site_core/component-failure/manifest.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/server/site_core/contradictory/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/server/site_core/contradictory/manifest.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/server/site_core/healthy/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/server/site_core/healthy/manifest.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/server/site_core/inbox-backlog/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/server/site_core/inbox-backlog/manifest.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/server/site_core/incomplete/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/server/site_core/incomplete/manifest.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/server/site_core/malformed/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/server/site_core/malformed/manifest.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/server/site_core/recovery/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/server/site_core/recovery/manifest.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/server/site_core/rotation-boundary/evidence/sccm/server/site-server/server-sitecomp/lo_/sitecomp.lo_crates/cmtraceopen-parser/tests/fixtures/sccm/server/site_core/rotation-boundary/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/server/site_core/rotation-boundary/manifest.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/server/site_core/status-processing-failure/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/server/site_core/status-processing-failure/manifest.jsoncrates/cmtraceopen-parser/tests/sccm_server_site_core.rscrates/cmtraceopen-parser/tests/sccm_site_core_fixture_contract.rs
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
🟢 Ready to approve
The changes are test- and fixture-driven, consistently align expected outputs with the updated schema, and include targeted regression coverage for the malformed-ID symmetry scenario described in the PR.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
- Files reviewed: 22/36 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
adamgell
marked this pull request as ready for review
August 2, 2026 11:23
This was referenced Aug 3, 2026
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.
Summary
.ok()?not-declaredgapsRoot cause
The earlier reducer could record declared-artifact coverage and then discard the corresponding finding when ID validation failed. That suppressed the synthetic missing-peer gap while also losing the malformed peer finding. The corrected reducer treats unsafe identifiers symmetrically and preserves a valid, deterministic diagnostic without fabricating facts or correlations.
Lineage
0b37e7a8..3a12cb8deecdb30f3a12cb8dVerification
wasm32-unknown-unknown: passedHosted CodeRabbit and GitHub Copilot reviews are requested after publication.
Relationship to existing work
Supersedes #405. Do not merge #405. That draft is closed without merge in favor of this corrected lineage.
This PR intentionally keeps #327 open; it does not close or fully resolve the issue.
Summary by CodeRabbit
New Features
Tests