feat(sccm): bind DP sources to sealed server intake - #458
Conversation
📝 WalkthroughWalkthroughAdds a canonical SCCM Distribution Point analyzer. It validates intake authority, artifact identity, metadata, topology, coverage, rotation, versions, and evidence. It emits deterministic observations, coverage gaps, and role-specific collection requests. Tests cover valid, invalid, incomplete, and reordered intake data. ChangesDistribution Point analysis
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SccmServerIntakeAssessment
participant analyze_distribution_point
participant SccmDistributionPointAnalysis
SccmServerIntakeAssessment->>analyze_distribution_point: provide intake assessment
analyze_distribution_point->>SccmServerIntakeAssessment: validate authority and canonical artifacts
analyze_distribution_point->>SccmDistributionPointAnalysis: emit observations, gaps, and requests
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 |
github-actions
Bot
added
enhancement
|
@coderabbitai full review — exact-head review requested for 63897dd. Please assess this only as the bounded sealed-intake DP source adapter described in the PR; it is not the full semantic DP reducer. |
|
I will assess the bounded sealed-intake DP source adapter only. I will not treat the absent semantic DP reducer as a defect in this PR. ✅ Action performedFull review finished. |
There was a problem hiding this comment.
Pull request overview
Adds the first SCCM server Distribution Point (DP) workflow slice by introducing a canonical-intake–bound adapter that projects only vetted DP distribution CCM evidence into a structured analysis output (observations, coverage gaps, and bounded artifact requests), without attempting any higher-level DP transaction/state reduction.
Changes:
- Introduces
analyze_distribution_point()adapter that enforces intake/topology seal authority and admits only canonical DP distribution sources/evidence. - Exposes the new adapter via
cmtraceopen_parser::sccm::server::windowsmodule exports. - Adds a dedicated integration test suite covering deterministic output, sealed-authority failure-closed behavior, coverage gap semantics, and bounded source requests.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| crates/cmtraceopen-parser/tests/sccm_server_distribution_point.rs | New integration tests validating the DP adapter’s canonical projection, determinism, and failure-closed/coverage-gap behavior. |
| crates/cmtraceopen-parser/src/sccm/server/windows/mod.rs | Wires the new distribution_point module into the Windows server surface and re-exports its API. |
| crates/cmtraceopen-parser/src/sccm/server/windows/distribution_point.rs | Implements the canonical-intake DP adapter, coverage-gap reasoning, and bounded artifact-request emission. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (8)
crates/cmtraceopen-parser/src/sccm/server/windows/distribution_point.rs (6)
540-573: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider cached or borrowed sort keys.
Both key functions allocate on every comparison.
source_observation_sort_keyclones twoStringvalues, andcoverage_gap_sort_keyallocates fourStringvalues plus aVec<String>clone.sort_byandsort_by_keycall these O(n log n) times.The collections are small, so this is not a current problem. If you want to remove the allocations, either return borrowed
&strtuples or usesort_by_cached_key, which computes each key once per element.🤖 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/distribution_point.rs` around lines 540 - 573, Update the sorting logic using source_observation_sort_key and coverage_gap_sort_key to avoid rebuilding allocated keys on every comparison. Prefer sort_by_cached_key so each element’s existing key function is evaluated once, preserving the current sort-key ordering and behavior.
221-241: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider one shared eligibility predicate and the short
SccmEvidencepath.Lines 225-228 and lines 238-241 repeat the same four eligibility checks. Extract them into one helper, for example
artifact_is_eligible(artifact), and call it from both places.Line 223 also spells out
crate::sccm::SccmEvidencealthough line 15 already importsSccmEvidence, andcanonical_evidence_setat line 375 uses the short path.♻️ Proposed refactor
+fn artifact_is_eligible(artifact: &SccmServerArtifactAssessment) -> bool { + artifact.state == SccmCoverageState::Captured + && artifact.profile_eligible + && artifact.parser_eligible + && artifact.fragment_complete != Some(false) +} + fn admitted_for_source_observation( artifact: &SccmServerArtifactAssessment, - evidence: &crate::sccm::SccmEvidence, + evidence: &SccmEvidence, ) -> bool { - artifact.state == SccmCoverageState::Captured - && artifact.profile_eligible - && artifact.parser_eligible - && artifact.fragment_complete != Some(false) + artifact_is_eligible(artifact) && evidence.role == artifact.producer_role && evidence.timestamp.ordering_state == SccmTimeOrderingState::NormalizedUtc }🤖 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/distribution_point.rs` around lines 221 - 241, Extract the four shared artifact eligibility checks from admitted_for_source_observation and artifact_metadata_is_congruent into an artifact_is_eligible helper, then reuse it in both predicates while preserving their additional conditions. Update admitted_for_source_observation to use the already imported SccmEvidence type instead of the fully qualified crate path.
355-370: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider precomputing an artifact index to remove repeated scans.
For every membership id, this closure scans
intake.artifactslinearly and then callsclassify_artifact_nametwice throughis_dp_distribution_artifactandrotation_is_canonical_for_artifact.coverage_is_congruentruns once per candidate artifact, so classification repeats many times for the same artifact.Intake bounds the artifact count and the coverage membership count, so the current cost stays bounded. If you want the adapter to stay cheap as those bounds grow, build one
BTreeMap<&str, &SccmServerArtifactAssessment>plus a cached DP-eligibility set inanalyze_distribution_pointand pass it down.🤖 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/distribution_point.rs` around lines 355 - 370, Optimize coverage matching by precomputing an artifact index in analyze_distribution_point: build a BTreeMap keyed by artifact ID plus a cached set of artifacts eligible for DP distribution, then pass both into coverage_is_congruent. Replace the repeated intake.artifacts scans and duplicate classification calls in the artifact_ids closure with indexed lookups while preserving all existing matching predicates and uniqueness checks.
184-196: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting the shared analysis envelope.
Lines 158-170 and lines 184-196 repeat the same
schema_version,workflow,profile, andcross_side_correlation_performedvalues. A single constructor keeps the two exit paths aligned when the profile or schema version changes.♻️ Proposed refactor
+fn analysis_envelope( + source_observations: Vec<SccmDistributionPointSourceObservation>, + coverage_gaps: Vec<SccmDistributionPointCoverageGap>, + artifact_requests: Vec<SccmArtifactRequest>, +) -> SccmDistributionPointAnalysis { + SccmDistributionPointAnalysis { + schema_version: SCCM_DISTRIBUTION_POINT_ANALYSIS_SCHEMA_VERSION, + workflow: SccmDistributionPointWorkflow::DistributionPointContent, + profile: SccmDistributionPointProfile { + id: SCCM_DISTRIBUTION_POINT_INTAKE_PROFILE_ID.to_owned(), + version: SCCM_DISTRIBUTION_POINT_INTAKE_PROFILE_VERSION, + stability: "experimental".to_owned(), + }, + source_observations, + coverage_gaps, + artifact_requests, + cross_side_correlation_performed: false, + } +}🤖 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/distribution_point.rs` around lines 184 - 196, Extract the repeated analysis envelope from the two construction paths in the relevant distribution-point analysis function into a shared constructor or helper. Centralize the common schema_version, workflow, profile, and cross_side_correlation_performed values, then have both paths reuse it while supplying their distinct source_observations, coverage_gaps, and artifact_requests fields.
255-277: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShare the profile-version predicate with intake.
distribution_point.rsrepeats the numeric5.00.BBBB.RRRRgrammar fromintake.rs. Extract the shared numeric predicate while preserving synthetic-only acceptance of"5.00.TEST". Opaque version handles are already excluded because they are notprofile_eligible.🤖 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/distribution_point.rs` around lines 255 - 277, Extract the shared numeric 5.00.BBBB.RRRR version predicate from supported_source_version into the existing intake-related shared symbol, then reuse it from distribution_point.rs. Keep "5.00.TEST" accepted only by the distribution-point predicate, while preserving rejection of opaque and malformed versions.
492-519: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winDefine canonical request identifiers in the catalog.
The catalog declares two logical names per producer role, but this function hardcodes only
distmgrandsmsDpProv. Add an explicit canonical request entry or shared helper so catalog changes cannot leave admission and follow-up requests inconsistent.🤖 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/distribution_point.rs` around lines 492 - 519, Update artifact_requests to obtain the distmgr and smsDpProv request identifiers from the catalog’s canonical request entries or shared helper instead of hardcoding string literals. Ensure every generated SccmArtifactRequest uses those canonical identifiers while preserving the existing producer-role routing and reasons.crates/cmtraceopen-parser/tests/sccm_server_distribution_point.rs (2)
92-104: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftPost-intake mutation makes the admission predicates untested.
Every one of these tests mutates the assessment after
assess_server_intakeseals it. Any such mutation breaksadapter_authority_is_intake_bound, soanalyze_distribution_pointreturns at line 89 ofcrates/cmtraceopen-parser/src/sccm/server/windows/distribution_point.rsbeforeis_dp_distribution_artifact,artifact_metadata_is_congruent,admitted_for_source_observation, orcanonical_evidence_setruns.assert_dp_coverage_onlyaccepts that outcome because it only checks non-emptiness, so the tests pass while the predicates they name stay uncovered. To cover a predicate, encode the defect in the fixture manifest so intake seals it, then assert the coverage-only projection.
crates/cmtraceopen-parser/tests/sccm_server_distribution_point.rs#L92-L104: assert the expected gap count,source_id,state,reason, and requestedlogical_idinstead of non-emptiness, so an authority-invalid result cannot satisfy a predicate-driven expectation.crates/cmtraceopen-parser/tests/sccm_server_distribution_point.rs#L421-L452: move the four defects ("profile-ineligible", "parser-ineligible", "incomplete-fragment", "invalid-evidence") into sealed fixture manifests soadmitted_for_source_observationandcanonical_evidence_setexecute, or rename the test to state that it only proves authority-gate determinism.crates/cmtraceopen-parser/tests/sccm_server_distribution_point.rs#L469-L477: add a fixture whose DP coverage omits the artifact at intake time, socoverage_is_congruentrejects the artifact instead of the authority gate.crates/cmtraceopen-parser/tests/sccm_server_distribution_point.rs#L479-L502: add sealed fixtures for the wrong subject role, the missing topology role, the ineligible profile, and the wrong rotation, sosubject_handle_is_congruent,topology_is_congruent, androtation_is_canonical_for_artifactexecute.Do you want me to open an issue to track the fixture-level admission coverage?
🤖 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_distribution_point.rs` around lines 92 - 104, Distribution-point tests mutate assessments after intake, so authority gating prevents the intended admission and congruence predicates from executing. In crates/cmtraceopen-parser/tests/sccm_server_distribution_point.rs:92-104, assert exact gap count, source_id, state, reason, and logical_id values rather than non-emptiness. At 421-452, encode the four defects in sealed fixture manifests so admitted_for_source_observation and canonical_evidence_set run, or rename the test to cover only authority-gate determinism. At 469-477, add a fixture omitting the artifact from DP coverage at intake so coverage_is_congruent rejects it; at 479-502, add sealed fixtures covering wrong subject role, missing topology role, ineligible profile, and wrong rotation for subject_handle_is_congruent, topology_is_congruent, and rotation_is_canonical_for_artifact.
13-15: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueImport
SccmServerIntakeAssessmentto shorten the signatures.The fully qualified path
cmtraceopen_parser::sccm::server::windows::SccmServerIntakeAssessmentappears six times, at lines 15, 41, 51, 61, 89, and 109. Line 3 already imports from the same module.♻️ Proposed refactor
use cmtraceopen_parser::sccm::server::windows::{ - analyze_distribution_point, assess_server_intake, SccmServerArtifactPayload, + analyze_distribution_point, assess_server_intake, SccmServerArtifactPayload, + SccmServerIntakeAssessment, };-fn load_assessment( - scenario: &str, -) -> cmtraceopen_parser::sccm::server::windows::SccmServerIntakeAssessment { +fn load_assessment(scenario: &str) -> SccmServerIntakeAssessment {🤖 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_distribution_point.rs` around lines 13 - 15, Import SccmServerIntakeAssessment from cmtraceopen_parser::sccm::server::windows alongside the existing module import, then replace all six fully qualified type references in load_assessment and the related test signatures with the imported name.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/cmtraceopen-parser/src/sccm/server/windows/distribution_point.rs`:
- Around line 540-573: Update the sorting logic using
source_observation_sort_key and coverage_gap_sort_key to avoid rebuilding
allocated keys on every comparison. Prefer sort_by_cached_key so each element’s
existing key function is evaluated once, preserving the current sort-key
ordering and behavior.
- Around line 221-241: Extract the four shared artifact eligibility checks from
admitted_for_source_observation and artifact_metadata_is_congruent into an
artifact_is_eligible helper, then reuse it in both predicates while preserving
their additional conditions. Update admitted_for_source_observation to use the
already imported SccmEvidence type instead of the fully qualified crate path.
- Around line 355-370: Optimize coverage matching by precomputing an artifact
index in analyze_distribution_point: build a BTreeMap keyed by artifact ID plus
a cached set of artifacts eligible for DP distribution, then pass both into
coverage_is_congruent. Replace the repeated intake.artifacts scans and duplicate
classification calls in the artifact_ids closure with indexed lookups while
preserving all existing matching predicates and uniqueness checks.
- Around line 184-196: Extract the repeated analysis envelope from the two
construction paths in the relevant distribution-point analysis function into a
shared constructor or helper. Centralize the common schema_version, workflow,
profile, and cross_side_correlation_performed values, then have both paths reuse
it while supplying their distinct source_observations, coverage_gaps, and
artifact_requests fields.
- Around line 255-277: Extract the shared numeric 5.00.BBBB.RRRR version
predicate from supported_source_version into the existing intake-related shared
symbol, then reuse it from distribution_point.rs. Keep "5.00.TEST" accepted only
by the distribution-point predicate, while preserving rejection of opaque and
malformed versions.
- Around line 492-519: Update artifact_requests to obtain the distmgr and
smsDpProv request identifiers from the catalog’s canonical request entries or
shared helper instead of hardcoding string literals. Ensure every generated
SccmArtifactRequest uses those canonical identifiers while preserving the
existing producer-role routing and reasons.
In `@crates/cmtraceopen-parser/tests/sccm_server_distribution_point.rs`:
- Around line 92-104: Distribution-point tests mutate assessments after intake,
so authority gating prevents the intended admission and congruence predicates
from executing. In
crates/cmtraceopen-parser/tests/sccm_server_distribution_point.rs:92-104, assert
exact gap count, source_id, state, reason, and logical_id values rather than
non-emptiness. At 421-452, encode the four defects in sealed fixture manifests
so admitted_for_source_observation and canonical_evidence_set run, or rename the
test to cover only authority-gate determinism. At 469-477, add a fixture
omitting the artifact from DP coverage at intake so coverage_is_congruent
rejects it; at 479-502, add sealed fixtures covering wrong subject role, missing
topology role, ineligible profile, and wrong rotation for
subject_handle_is_congruent, topology_is_congruent, and
rotation_is_canonical_for_artifact.
- Around line 13-15: Import SccmServerIntakeAssessment from
cmtraceopen_parser::sccm::server::windows alongside the existing module import,
then replace all six fully qualified type references in load_assessment and the
related test signatures with the imported name.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: cc49bf41-2a5c-45df-a666-28ed1f4585e7
📒 Files selected for processing (3)
crates/cmtraceopen-parser/src/sccm/server/windows/distribution_point.rscrates/cmtraceopen-parser/src/sccm/server/windows/mod.rscrates/cmtraceopen-parser/tests/sccm_server_distribution_point.rs
|
Published test-only correction 2150f85 after independently validating CodeRabbit review feedback. Production is unchanged. The 16 DP tests now separate three reachable valid-seal adapter guards (profile eligibility, missing subject handle, unobserved DP role), intake-level role/rotation rejection, and explicit post-intake authority quarantine for mutations that cannot exist in canonical sealed intake. Focused 16/16, full parser, strict Clippy, wasm32, TypeScript build, scoped Rustfmt/diff, and local CodeRabbit zero findings pass. Fresh exact-head hosted CodeRabbit, Copilot, and CI remain merge gates. |
|
@coderabbitai full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 17 minutes. |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
crates/cmtraceopen-parser/src/sccm/server/windows/distribution_point.rs (3)
221-253: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared eligibility predicate.
admitted_for_source_observationandartifact_metadata_is_congruentrepeat the same four artifact checks:state == Captured,profile_eligible,parser_eligible, andfragment_complete != Some(false). A future change to one list can silently diverge from the other.♻️ Proposed extraction
+fn artifact_is_usable(artifact: &SccmServerArtifactAssessment) -> bool { + artifact.state == SccmCoverageState::Captured + && artifact.profile_eligible + && artifact.parser_eligible + && artifact.fragment_complete != Some(false) +} + fn admitted_for_source_observation( artifact: &SccmServerArtifactAssessment, evidence: &crate::sccm::SccmEvidence, ) -> bool { - artifact.state == SccmCoverageState::Captured - && artifact.profile_eligible - && artifact.parser_eligible - && artifact.fragment_complete != Some(false) + artifact_is_usable(artifact) && evidence.role == artifact.producer_role && evidence.timestamp.ordering_state == SccmTimeOrderingState::NormalizedUtc }🤖 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/distribution_point.rs` around lines 221 - 253, Extract the repeated artifact eligibility checks from admitted_for_source_observation and artifact_metadata_is_congruent into a shared predicate, then call it from both functions. Preserve the existing four conditions exactly and leave each function’s additional evidence, source-version, topology, and coverage checks unchanged.
355-370: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueReduce repeated classification work in the coverage check.
coverage_is_congruentruns for each candidate artifact. For every coverage membership it scans allintake.artifactsand callsis_dp_distribution_artifactandrotation_is_canonical_for_artifact, which each callclassify_artifact_nameand scandeclared_server_source_catalog(). The total work grows quadratically with artifact count. Manifest limits bound the input, so this is not a runtime risk today. Precompute a map fromartifact_idto the DP-eligibility and rotation results once inanalyze_distribution_point, then look values up here.🤖 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/distribution_point.rs` around lines 355 - 370, In analyze_distribution_point, precompute and retain a map keyed by artifact_id containing each artifact’s DP-eligibility and canonical-rotation results, so classification and declared_server_source_catalog scans occur only once per artifact. Update coverage_is_congruent’s artifact membership check to use this map alongside the existing identity and uniqueness checks, removing the repeated intake.artifacts scan and calls to is_dp_distribution_artifact and rotation_is_canonical_for_artifact.
255-277: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the intake version predicate instead of duplicating it.
supported_source_versionre-implementssource_version_is_profile_eligiblefromintake.rs(lines 2365-2384). Two differences exist. This copy accepts"5.00.TEST"unconditionally, while intake accepts it only for synthetic fixtures. Sealed intake currently blocks that value for non-synthetic manifests, so the adapter stays closed today, but the two predicates can drift apart later. Export a shared helper in the module and call it from both sites.🤖 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/distribution_point.rs` around lines 255 - 277, Replace the duplicated validation in supported_source_version with a shared exported helper from intake.rs, based on source_version_is_profile_eligible. Update both the intake call site and supported_source_version to use that helper, preserving the synthetic-fixture handling for "5.00.TEST" so both paths enforce identical eligibility rules.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/cmtraceopen-parser/src/sccm/server/windows/distribution_point.rs`:
- Around line 221-253: Extract the repeated artifact eligibility checks from
admitted_for_source_observation and artifact_metadata_is_congruent into a shared
predicate, then call it from both functions. Preserve the existing four
conditions exactly and leave each function’s additional evidence,
source-version, topology, and coverage checks unchanged.
- Around line 355-370: In analyze_distribution_point, precompute and retain a
map keyed by artifact_id containing each artifact’s DP-eligibility and
canonical-rotation results, so classification and declared_server_source_catalog
scans occur only once per artifact. Update coverage_is_congruent’s artifact
membership check to use this map alongside the existing identity and uniqueness
checks, removing the repeated intake.artifacts scan and calls to
is_dp_distribution_artifact and rotation_is_canonical_for_artifact.
- Around line 255-277: Replace the duplicated validation in
supported_source_version with a shared exported helper from intake.rs, based on
source_version_is_profile_eligible. Update both the intake call site and
supported_source_version to use that helper, preserving the synthetic-fixture
handling for "5.00.TEST" so both paths enforce identical eligibility rules.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0ecdb59e-0528-46e4-bdc2-c0b8b854b61c
📒 Files selected for processing (3)
crates/cmtraceopen-parser/src/sccm/server/windows/distribution_point.rscrates/cmtraceopen-parser/src/sccm/server/windows/mod.rscrates/cmtraceopen-parser/tests/sccm_server_distribution_point.rs
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (2)
crates/cmtraceopen-parser/src/sccm/server/windows/distribution_point.rs:555
coverage_gap_sort_keyclones multipleStrings and theVec<String>just to sort coverage gaps. This adds avoidable allocations and copies; returning borrowed strings/slices keeps the sort deterministic without extra heap work.
fn coverage_gap_sort_key(
gap: &SccmDistributionPointCoverageGap,
) -> (String, String, String, String, Vec<String>) {
(
gap.source_id.clone(),
crates/cmtraceopen-parser/src/sccm/server/windows/distribution_point.rs:549
source_observation_sort_keyclonesartifact_idandentry_idjust to build a sort key. This allocates during sorting and can become noticeable when a DP artifact has many evidence records. Prefer returning borrowed&strfields for the key to keep sorting allocation-free.
This issue also appears on line 551 of the same file.
fn source_observation_sort_key(
observation: &SccmDistributionPointSourceObservation,
) -> (String, u32, u32, String) {
(
observation.artifact_id.clone(),
|
Independent disposition of the exact-head review nits: (1) the duplicated four-condition eligibility predicates are currently identical and covered by the 16-test matrix; extracting them is a maintenance follow-up, not a semantic defect. (2) repeated DP classification is bounded by the 512-artifact intake ceiling and CodeRabbit itself identifies no current runtime risk; precomputation belongs with the full semantic reducer. (3) the local version-shape recheck cannot admit non-synthetic 5.00.TEST because both adapter/topology seals and intake profile eligibility are required; sharing the intake helper would need synthetic provenance that the public assessment intentionally does not expose and would overlap the active shared SUP intake seam. (4) Copilot sort-key allocation observations are valid performance cleanup before expanding to high-volume semantic facts, but do not change deterministic output or authority. No review suggestion is being silently accepted or dismissed; these are recorded gates for the next #329 reducer slice, while this PR remains the bounded unwired source adapter. |
…hority-restack-r115
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (1)
crates/cmtraceopen-parser/src/sccm/server/windows/distribution_point.rs:496
artifact_requestsonly ever emitsdistmgrand/orsmsDpProvrequests, even though the declared DP distribution source catalog also includespkgXferMgr(site-side) andpullDp(DP-side). If intake coverage gaps occur for those other declared DP distribution artifacts, the adapter will request the wrong logical artifact (or fail to request a compatible one), making recollection guidance inaccurate.
Consider generating requests from the declared source spec(s) for server-dp-distribution by producer role (or from the specific artifact IDs present in the gap), so the requested logical IDs match the eligible DP-distribution sources actually in play (e.g., include pkgXferMgr/pullDp where applicable).
fn artifact_requests(gaps: &[SccmDistributionPointCoverageGap]) -> Vec<SccmArtifactRequest> {
let mut requests = Vec::with_capacity(gaps.len());
for gap in gaps {
if gap.producer_role.is_none() {
requests.push(SccmArtifactRequest {
There was a problem hiding this comment.
🧹 Nitpick comments (3)
crates/cmtraceopen-parser/src/sccm/server/windows/distribution_point.rs (2)
221-224: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the imported
SccmEvidencepath.
SccmEvidenceis imported at line 15.canonical_evidence_setat line 375 uses the short path. Use the same form here for consistency.♻️ Proposed change
fn admitted_for_source_observation( artifact: &SccmServerArtifactAssessment, - evidence: &crate::sccm::SccmEvidence, + evidence: &SccmEvidence, ) -> bool {🤖 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/distribution_point.rs` around lines 221 - 224, Update the admitted_for_source_observation function signature to use the imported SccmEvidence type directly instead of the fully qualified crate::sccm::SccmEvidence path, matching canonical_evidence_set and the existing import.
255-277: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueReuse the canonical source-version predicate.
Move
supported_source_versioninto shared intake logic and use it from the Distribution Point adapter. This prevents future drift betweensynthetic_fixturehandling and profile eligibility.🤖 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/distribution_point.rs` around lines 255 - 277, Move the supported_source_version predicate into the shared intake logic, then update the Distribution Point adapter to import and call that canonical helper instead of defining its own local version. Ensure both synthetic_fixture handling and profile eligibility reuse the same predicate.crates/cmtraceopen-parser/tests/sccm_server_distribution_point.rs (1)
111-195: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd assertions for
schema_versionandprofile.No test in this file asserts
analysis.schema_version,analysis.profile.id,analysis.profile.version, oranalysis.profile.stability. Those four fields are part of the public analysis contract thatdistribution_point.rsdeclares at lines 23-25 and 158-165. A change to any of them would pass all 12 tests in this file.Pin them in one place, for example inside both shared helpers or in the canonical happy-path test.
💚 Proposed addition
fn assert_dp_intake_authority_invalid( assessment: &SccmServerIntakeAssessment, context: &str, ) -> Value { let analysis = analyze_distribution_point(assessment); + assert_eq!(analysis.schema_version, 1, "{context}"); + assert_eq!(analysis.profile.id, "sccm-dp-intake-envelope", "{context}"); + assert_eq!(analysis.profile.version, 1, "{context}"); + assert_eq!(analysis.profile.stability, "experimental", "{context}"); assert!(🤖 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_distribution_point.rs` around lines 111 - 195, Add assertions covering analysis.schema_version and analysis.profile.id, analysis.profile.version, and analysis.profile.stability in the shared distribution-point test helpers, preferably assert_dp_sealed_guard_rejection and assert_dp_intake_authority_invalid so all related cases are pinned to the public contract. Use the canonical expected values declared by distribution_point.rs and keep the existing validation unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/cmtraceopen-parser/src/sccm/server/windows/distribution_point.rs`:
- Around line 221-224: Update the admitted_for_source_observation function
signature to use the imported SccmEvidence type directly instead of the fully
qualified crate::sccm::SccmEvidence path, matching canonical_evidence_set and
the existing import.
- Around line 255-277: Move the supported_source_version predicate into the
shared intake logic, then update the Distribution Point adapter to import and
call that canonical helper instead of defining its own local version. Ensure
both synthetic_fixture handling and profile eligibility reuse the same
predicate.
In `@crates/cmtraceopen-parser/tests/sccm_server_distribution_point.rs`:
- Around line 111-195: Add assertions covering analysis.schema_version and
analysis.profile.id, analysis.profile.version, and analysis.profile.stability in
the shared distribution-point test helpers, preferably
assert_dp_sealed_guard_rejection and assert_dp_intake_authority_invalid so all
related cases are pinned to the public contract. Use the canonical expected
values declared by distribution_point.rs and keep the existing validation
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 519dbff3-ba9a-4f36-9661-34c828244e43
📒 Files selected for processing (3)
crates/cmtraceopen-parser/src/sccm/server/windows/distribution_point.rscrates/cmtraceopen-parser/src/sccm/server/windows/mod.rscrates/cmtraceopen-parser/tests/sccm_server_distribution_point.rs
|
Fresh exact-head review disposition:
No review finding authorizes a client causal claim, a role diagnosis, native collection, or a broad parser. The issue remains open and the semantic reducer branch may not expand until those accepted hardening contracts pass independent review. |
Reviewed bounded adapter slice only. The full package/content/version reducer, correlation, native collection, and live Windows validation remain open.
Scope
Implements the first production slice of the Distribution Point workflow: a pure-Rust, wasm-compatible adapter that projects only canonical
server-dp-distributionlogical CCM evidence from the reviewed server-intake boundary.This deliberately does not complete the Distribution Point diagnostic workflow. It emits cited source observations, explicit coverage gaps, and bounded source requests only. It does not yet reduce package/content/version transactions, emit terminal DP findings, perform native collection, or perform client/server correlation. The tracking issue remains open.
Authority and safety contract
distmgr.logand DP-sideSMSDPProv.logwhen the missing side is not yet known.cross_side_correlation_performedis always false.Test-first history
The branch preserves separate RED and GREEN commits for canonical intake behavior, canonical evidence and coverage continuity, sealed adapter/topology authority, dual-source requests for unscoped gaps, stable serialized coverage reason labels, and the distinction between sealed lower-predicate tests and post-intake authority quarantine.
Verification at current integration
Head:
0e63f6a09e61074f0e32c3745e2acb3921975815Base:
a185a68fe6c1290aece9d55805e4173262a61193Merge tree:
77ee09a69542e959715c52fed1c895af95558bedGitHub updated the branch without force. The new merge commit has parents
2150f85danda185a68f; its tree is byte-identical to the independently validated current-base synthetic merge.npx tsc --noEmit: passrustfmt --checkon all three changed files: passgit diff --check: passRepository-wide
cargo fmt --check --allreports inherited drift only in five non-#329 ESP files; no changed file overlaps those hunks.Fresh hosted CodeRabbit, Copilot, and CI are required on this exact head before merge. Prior hosted reviews are not treated as current-head acceptance.
Remaining dependency/state
The full package/content/version/DP state reducer remains required and will consume the already-merged ten-scenario synthetic corpus after this source boundary lands. Content-to-DP correlation remains blocked until both upstream semantic fact contracts are stable. Native Windows/SCCM validation remains future work; this PR makes no live-acceptance claim.
Closes no issue.
Summary by CodeRabbit