fix(sccm): bind Management Point analysis to canonical intake - #443
Conversation
📝 WalkthroughWalkthroughThe PR adds integrity-bound SCCM server intake and a validated Management Point analysis entry point. It adds typed intake errors, bounded SHA-256 serialization, canonical fixture loading, source-version updates, and extensive evidence and coverage tests. ChangesManagement Point server-intake integration
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant SccmServerArtifactPayload
participant assess_server_intake
participant analyze_management_point_from_server_intake
participant analyze_management_point_fixture
SccmServerArtifactPayload->>assess_server_intake: normalized artifacts and metadata
assess_server_intake->>analyze_management_point_from_server_intake: integrity-bound assessment
analyze_management_point_from_server_intake->>analyze_management_point_fixture: admitted Management Point bundle
analyze_management_point_fixture-->>analyze_management_point_from_server_intake: Management Point analysis
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Comment |
github-actions
Bot
added
bug
|
Exact-head review request for @coderabbitai review Please review the complete PR range and generated merge ref against current target This is synthetic-profile parser validation only. There is no native or live Windows/SCCM acceptance claim. The reader landed first; the draft remains behind the subsequent discovery -> capture merge-order gates. |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
head_commit="bae0255f581d0e6596c1cb23b538184b4f7cdc5b"
target_commit="431909709ae3e66bb4c236647af92bd5312bcc90"
branch_point="94195dcca72df0dcdaf33a983b5821710fd369a7"
printf '%s\n' '== Commit objects =='
git cat-file -t "$head_commit"
git cat-file -t "$target_commit"
git cat-file -t "$branch_point"
printf '%s\n' '== Commit identity and parents =='
git show -s --format='%H%nParents: %P%nSubject: %s' "$head_commit"
git show -s --format='%H%nParents: %P%nSubject: %s' "$target_commit"
git merge-base "$head_commit" "$target_commit"
printf '%s\n' '== Three-dot changed files and summary =='
git diff --stat "$branch_point...$head_commit"
git diff --name-status "$branch_point...$head_commit"
printf '%s\n' '== Target-to-head changed files and summary =='
git diff --stat "$target_commit...$head_commit"
git diff --name-status "$target_commit...$head_commit"
printf '%s\n' '== Generated merge-tree result =='
git merge-tree "$target_commit" "$head_commit" | sed -n '1,260p'
printf '%s\n' '== Exact PR diff with zero context =='
git diff --no-ext-diff --unified=0 "$branch_point...$head_commit"Length of output: 148958 🏁 Script executed: #!/bin/bash
set -euo pipefail
head_commit="bae0255f581d0e6596c1cb23b538184b4f7cdc5b"
merge_tree="12923b8b68e2960b804d4119c82645b2733055d4"
intake="crates/cmtraceopen-parser/src/sccm/server/windows/intake.rs"
mp="crates/cmtraceopen-parser/src/sccm/server/windows/management_point.rs"
integration="crates/cmtraceopen-parser/tests/sccm_server_management_point.rs"
printf '%s\n' '== Module outlines at exact head =='
git show "$head_commit:$intake" > /tmp/intake.rs
git show "$head_commit:$mp" > /tmp/management_point.rs
git show "$head_commit:$integration" > /tmp/sccm_server_management_point.rs
ast-grep outline /tmp/intake.rs --items all --type struct,enum,impl,function --match 'SccmServerIntakeAssessment|assess_server_intake|canonical_evidence_integrity|payload_sha256'
ast-grep outline /tmp/management_point.rs --items all --type struct,enum,impl,function --match 'analyze_management_point|SccmManagementPointIntakeError|parse_fact|valid_management_point_handle|SccmManagementPointBundle'
printf '%s\n' '== Intake assessment, construction, and integrity implementation =='
grep -n -E 'struct SccmServerIntakeAssessment|impl SccmServerIntakeAssessment|fn assess_server_intake|fn canonical_evidence_integrity|fn payload_sha256' /tmp/intake.rs
sed -n '35,220p' /tmp/intake.rs
sed -n '500,590p' /tmp/intake.rs
sed -n '1010,1110p' /tmp/intake.rs
printf '%s\n' '== Management Point adapter and reducer entry points =='
grep -n -E 'pub (fn|enum|struct)|fn analyze_management_point|fn parse_fact|fn validate|fn valid_management_point_handle|fn opaque_host_handle' /tmp/management_point.rs
sed -n '1,135p' /tmp/management_point.rs
sed -n '260,520p' /tmp/management_point.rs
sed -n '990,1335p' /tmp/management_point.rs
printf '%s\n' '== Exact symbol call sites and re-exports =='
rg -n -C 3 --glob '*.rs' \
'analyze_management_point(_fixture|_from_server_intake)?\s*\(|SccmManagementPointIntakeError|evidence_projection_is_intake_bound' \
crates/cmtraceopen-parser
printf '%s\n' '== Direct Windows/native dependencies in changed production modules =='
rg -n -i -C 2 --glob '*.rs' \
'std::os::windows|windows(_sys|::)|winapi|windows-sys|#[[:space:]]*\[cfg\(target_os[[:space:]]*=[[:space:]]*"windows"\)\]' \
"$intake" "$mp" || true
printf '%s\n' '== Generated merge tree source equivalence for changed production files =='
for path in "$intake" "$mp"; do
printf '%s\n' "-- $path"
git diff --no-ext-diff --exit-code \
<(git show "$head_commit:$path") \
<(git show "$merge_tree:$path") || true
done
printf '%s\n' '== Public integration test at exact head =='
cat -n /tmp/sccm_server_management_point.rsLength of output: 3006 🏁 Script executed: #!/bin/bash
set -euo pipefail
head_commit="bae0255f581d0e6596c1cb23b538184b4f7cdc5b"
merge_tree="12923b8b68e2960b804d4119c82645b2733055d4"
intake="crates/cmtraceopen-parser/src/sccm/server/windows/intake.rs"
mp="crates/cmtraceopen-parser/src/sccm/server/windows/management_point.rs"
integration="crates/cmtraceopen-parser/tests/sccm_server_management_point.rs"
git show "$head_commit:$intake" > /tmp/intake.rs
git show "$head_commit:$mp" > /tmp/management_point.rs
git show "$head_commit:$integration" > /tmp/sccm_server_management_point.rs
printf '%s\n' '== Module outlines at exact head =='
ast-grep outline /tmp/intake.rs --items all --match 'SccmServerIntakeAssessment|assess_server_intake|canonical_evidence_integrity|payload_sha256'
ast-grep outline /tmp/management_point.rs --items all --match 'analyze_management_point|SccmManagementPointIntakeError|parse_fact|valid_management_point_handle|SccmManagementPointBundle'
printf '%s\n' '== Intake assessment, construction, and integrity implementation =='
grep -n -E 'struct SccmServerIntakeAssessment|impl SccmServerIntakeAssessment|fn assess_server_intake|fn canonical_evidence_integrity|fn payload_sha256' /tmp/intake.rs
sed -n '35,220p' /tmp/intake.rs
sed -n '500,590p' /tmp/intake.rs
sed -n '1010,1110p' /tmp/intake.rs
printf '%s\n' '== Management Point adapter and reducer entry points =='
grep -n -E 'pub (fn|enum|struct)|fn analyze_management_point|fn parse_fact|fn validate|fn valid_management_point_handle|fn opaque_host_handle' /tmp/management_point.rs
sed -n '1,135p' /tmp/management_point.rs
sed -n '260,520p' /tmp/management_point.rs
sed -n '990,1335p' /tmp/management_point.rs
printf '%s\n' '== Exact symbol call sites and re-exports =='
rg -n -C 3 --glob '*.rs' \
'analyze_management_point(_fixture|_from_server_intake)?\s*\(|SccmManagementPointIntakeError|evidence_projection_is_intake_bound' \
crates/cmtraceopen-parser
printf '%s\n' '== Direct Windows/native dependencies in changed production modules =='
rg -n -i -C 2 --glob '*.rs' \
'std::os::windows|windows(_sys|::)|winapi|windows-sys|#[[:space:]]*\[cfg\(target_os[[:space:]]*=[[:space:]]*"windows"\)\]' \
"$intake" "$mp" || true
printf '%s\n' '== Generated merge tree source equivalence for changed production files =='
for path in "$intake" "$mp"; do
printf '%s\n' "-- $path"
if git diff --no-ext-diff --exit-code \
<(git show "$head_commit:$path") \
<(git show "$merge_tree:$path"); then
echo "identical"
fi
done
printf '%s\n' '== Public integration test at exact head =='
cat -n /tmp/sccm_server_management_point.rsLength of output: 89593 🏁 Script executed: #!/bin/bash
set -euo pipefail
head_commit="bae0255f581d0e6596c1cb23b538184b4f7cdc5b"
intake="crates/cmtraceopen-parser/src/sccm/server/windows/intake.rs"
mp="crates/cmtraceopen-parser/src/sccm/server/windows/management_point.rs"
mp_tests="crates/cmtraceopen-parser/src/sccm/server/windows/management_point_tests.rs"
git show "$head_commit:$intake" > /tmp/intake.rs
git show "$head_commit:$mp" > /tmp/management_point.rs
git show "$head_commit:$mp_tests" > /tmp/management_point_tests.rs
printf '%s\n' '== Topology and artifact assessment definitions =='
grep -n -E 'struct SccmServerTopologyAssessment|struct SccmServerArtifactAssessment|fn prepare_artifact|fn assess_topology|site_handle|roles_observed|source_version' /tmp/intake.rs
sed -n '250,405p' /tmp/intake.rs
sed -n '590,845p' /tmp/intake.rs
printf '%s\n' '== Reducer checks for topology and Management Point handle equality =='
grep -n -E 'management_point_host_handle|topology_site_code|fact\.site_code|source_is_admitted|evidence_reference_fits_source|fragment_complete' /tmp/management_point.rs
sed -n '475,760p' /tmp/management_point.rs
sed -n '1320,1925p' /tmp/management_point.rs
printf '%s\n' '== Tests that mutate adapter-authoritative intake fields =='
rg -n -C 5 \
'roles_observed|site_handle|producer_host_handle|source_id|source_version|original_basename|coverage|artifact.*(push|retain)|topology' \
/tmp/management_point_tests.rs \
crates/cmtraceopen-parser/tests/sccm_server_management_point.rs
printf '%s\n' '== Binding scope and assessment construction sites =='
rg -n -C 3 \
'SccmServerIntakeAssessment\s*\{|evidence_integrity|evidence_projection_is_intake_bound' \
crates/cmtraceopen-parser
printf '%s\n' '== Dependencies relevant to wasm32 in parser crate manifest =='
fd -a '^Cargo\.toml$' crates/cmtraceopen-parser -x sh -c \
'echo "== $1 =="; sed -n "/^\[dependencies\]/,/^\[/p;/^\[target\./,/^\[/p" "$1"' sh {}Length of output: 89584 🏁 Script executed: #!/bin/bash
set -euo pipefail
head_commit="bae0255f581d0e6596c1cb23b538184b4f7cdc5b"
intake="crates/cmtraceopen-parser/src/sccm/server/windows/intake.rs"
git show "$head_commit:$intake" > /tmp/intake.rs
printf '%s\n' '== Source version and artifact eligibility logic =='
sed -n '790,1065p' /tmp/intake.rs
sed -n '1440,1585p' /tmp/intake.rs
printf '%s\n' '== Intake fixture manifests with fragment, truncation, profile, or coverage metadata =='
fd -a '^manifest\.json$' crates/cmtraceopen-parser/tests/fixtures/sccm/server \
-x sh -c '
if rg -n -i "\"(fragmentComplete|limitApplied|sourceVersion|captureState|producerRole|sourceId)\"" "$1" >/dev/null; then
echo "== $1 =="
rg -n -i -C 2 "\"(fragmentComplete|limitApplied|sourceVersion|captureState|producerRole|sourceId)\"" "$1"
fi
' sh {} | sed -n '1,900p'
printf '%s\n' '== Exact incomplete/profile-related server intake test cases =='
rg -n -i -C 5 \
'fragment_complete|fragmentComplete|truncated|limit_applied|profile_eligible|source_version|5\.00\.TEST|Captured' \
crates/cmtraceopen-parser/src/sccm/server/windows/intake.rs \
crates/cmtraceopen-parser/tests/sccm_server_intake.rs \
crates/cmtraceopen-parser/tests/sccm_server_site_core.rs \
crates/cmtraceopen-parser/tests/fixtures/sccm/server
printf '%s\n' '== Exact adapter checks and missing positive-mutation tests =='
rg -n -C 3 \
'IncompleteSource|ProfileMismatch|fragment_complete\s*=|truncated\s*=|parser_eligible\s*=|profile_eligible\s*=|state\s*=' \
crates/cmtraceopen-parser/src/sccm/server/windows/management_point_tests.rs \
crates/cmtraceopen-parser/tests/sccm_server_management_point.rsLength of output: 202186
P1 — Bind the complete adapter input, not only evidence. A caller can clone a canonical assessment, keep its evidence unchanged, and rewrite source-admission metadata such as Bind a canonical projection that includes every adapter-authoritative topology, artifact, coverage, and evidence field. Alternatively, make those fields immutable outside canonical intake. Add mutation tests that promote an incomplete or profile-ineligible source to admitted metadata while preserving the original evidence vector. The adapter must reject those inputs. I confirmed that the requested head merges without conflicts with ✅ Action performedReview finished.
|
There was a problem hiding this comment.
Pull request overview
This PR hardens SCCM server Management Point (MP) diagnostics by making MP reduction depend on a canonical SccmServerIntakeAssessment, and by adding an integrity binding so downstream reducers can detect any caller-rewritten evidence (line ranges, ownership, message/timestamp, duplicates/collisions) and fail closed.
Changes:
- Adds a public, fallible entry point
analyze_management_point_from_server_intake(...)that only reduces MP evidence after canonical server intake validation. - Extends canonical server intake to compute and retain a private SHA-256 integrity map for normalized evidence, enabling reducers to verify the evidence projection is still intake-bound.
- Refactors and expands MP tests: legacy bundle-based reducer tests are moved under
#[cfg(test)], and a new canonical-intake fixture + adapter test are added.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| crates/cmtraceopen-parser/src/sccm/server/windows/management_point.rs | Adds the canonical-intake MP entry point and new intake error type; updates MP handle validation. |
| crates/cmtraceopen-parser/src/sccm/server/windows/intake.rs | Adds private evidence integrity binding and an API to verify the evidence projection is still intake-bound. |
| crates/cmtraceopen-parser/src/sccm/server/windows/management_point_tests.rs | Introduces a large in-crate test suite covering fixture reducer behavior and canonical intake adapter fail-closed behavior. |
| crates/cmtraceopen-parser/tests/sccm_server_management_point.rs | Simplifies the integration test to focus on the canonical-intake adapter and a fail-closed truncation case. |
| crates/cmtraceopen-parser/tests/fixtures/sccm/server/management-point/canonical-intake-policy-scope/manifest.json | Adds a canonical server-intake MP fixture manifest used by the adapter tests. |
| crates/cmtraceopen-parser/tests/fixtures/sccm/server/management-point/canonical-intake-policy-scope/evidence/sccm/server/management-point/server-mp-policy/current/MP_GetPolicy.log | Adds the minimal MP log payload for the canonical intake fixture. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
crates/cmtraceopen-parser/src/sccm/server/windows/management_point_tests.rs (3)
187-189: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueParse the manifest string that is already in memory.
Lines 187 and 189 read
manifest.jsontwice from disk. Parsemanifest_jsondirectly instead.♻️ Proposed change
let manifest_json = fs::read_to_string(directory.join("manifest.json")) .expect("server intake fixture manifest must be readable"); - let manifest = load_json(&directory.join("manifest.json")); + let manifest: Value = serde_json::from_str(&manifest_json) + .expect("server intake fixture manifest must be valid JSON");🤖 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/management_point_tests.rs` around lines 187 - 189, Update the manifest loading in the test setup to pass the already-read manifest_json string to load_json instead of constructing another path and reading manifest.json again.
607-617: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert that the supplemental IIS artifact changes nothing in the output.
Line 617 checks only that the call succeeds. The stated contract is that a skipped, parser-ineligible, non-MP source is ignored.
Compare the analysis against the baseline to prove that the supplemental artifact contributed no observation, no evidence, and no coverage gap.
💚 Stronger assertion
- assert!(analyze_management_point_from_server_intake(&supplemental_iis).is_ok()); + let supplemental_analysis = analyze_management_point_from_server_intake(&supplemental_iis) + .expect("a skipped non-MP source must not block admission"); + assert_eq!( + supplemental_analysis, analysis, + "a skipped supplemental IIS source must not change the MP analysis" + );🤖 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/management_point_tests.rs` around lines 607 - 617, Update the test around analyze_management_point_from_server_intake to retain the baseline analysis result before adding the supplemental IIS artifact, then assert the supplemental result is identical to that baseline across observations, evidence, and coverage gaps. Keep the existing skipped, parser-ineligible artifact setup unchanged and replace the success-only assertion with a direct output comparison.
694-697: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the specific error variant in the forged-evidence tests.
These assertions use a bare
.is_err(). The neighbouring test at lines 624-679 usesmatches!against a named variant.A bare
.is_err()passes when the adapter rejects for any reason. If a later change causes rejection at an earlier gate, for example the topology check, these tests still pass and stop proving that forged evidence is what was rejected.Match
Err(SccmManagementPointIntakeError::SourceMismatch { .. })or the variant each case expects.Also applies to: 706-709, 713-716, 727-730, 734-737, 746-749, 755-758
🤖 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/management_point_tests.rs` around lines 694 - 697, Update the forged-evidence assertions in analyze_management_point_from_server_intake and the listed neighboring cases to use matches! against Err(SccmManagementPointIntakeError::SourceMismatch { .. }) (or each case’s expected named variant) instead of bare is_err(), preserving the existing failure messages.crates/cmtraceopen-parser/src/sccm/server/windows/management_point.rs (1)
377-382: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAlign the
physical_line_endcomputation with the admitted-evidence filter.Lines 377-382 compute
physical_line_endover every evidence row for the artifact. Lines 423-431 then admit only rows whereevidence.role == SccmRole::ManagementPoint.An evidence row with a non-ManagementPoint role therefore widens the physical line bound while never entering the bundle. The reducer uses
physical_line_endas the upper bound for citation-fit checks, so the bound becomes looser than the evidence that must satisfy it.Apply the same role predicate in both places.
♻️ Proposed alignment
let physical_line_end = assessment .evidence .iter() - .filter(|evidence| evidence.reference.artifact_id == artifact.artifact_id) + .filter(|evidence| { + evidence.role == SccmRole::ManagementPoint + && evidence.reference.artifact_id == artifact.artifact_id + }) .filter_map(|evidence| evidence.reference.line_end) .max();🤖 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/management_point.rs` around lines 377 - 382, Update the physical_line_end computation near assessment.evidence to also filter evidence.role == SccmRole::ManagementPoint, matching the admitted-evidence filter used later in the bundle construction. Keep the existing artifact_id and line_end filtering and max aggregation unchanged.crates/cmtraceopen-parser/tests/sccm_server_management_point.rs (1)
11-43: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe fixture loader duplicates the in-crate helper.
Lines 11-43 repeat
FIXTURE_ROOT,fixture_directory, and the payload-building logic frommanagement_point_tests.rslines 20, 77-81, and 184-208.An integration test must use the public API, so some duplication is structural. You can still remove most of it by exposing the loader from a shared
tests/common/module, or by exporting a#[cfg(feature = "test-fixtures")]helper from the crate.Keep this file. It is the only test that proves the new entry point is reachable from outside the crate.
🤖 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_management_point.rs` around lines 11 - 43, Remove the duplicated fixture-loading logic from canonical_intake and reuse a shared test fixture loader, such as a tests/common helper or the crate’s test-fixture API, for FIXTURE_ROOT, fixture_directory, manifest parsing, and payload construction. Preserve this integration test and its external assess_server_intake call so it continues validating the public entry point.
🤖 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/intake.rs`:
- Around line 195-199: Update evidence_projection_is_intake_bound to validate a
canonical intake projection containing all adapter-authoritative topology,
artifacts, coverage, and evidence fields, rather than evidence alone. Ensure
mutations to public topology, artifacts, or coverage invalidate the binding and
cause Management Point admission to reject incomplete or profile-ineligible
sources, while preserving acceptance for unchanged canonical assessments; add
mutation tests covering these cases.
In `@crates/cmtraceopen-parser/src/sccm/server/windows/management_point.rs`:
- Around line 1301-1316: Gate the "synthetic:host:mp-01" branch in
valid_management_point_handle behind the canonical intake’s synthetic-fixture
flag, threading that flag through callers as needed so production bundles cannot
use it. Preserve valid_safe_handle and opaque_host_handle behavior, and simplify
the hexadecimal-byte check to use matches!(byte, b'a'..=b'f').
- Around line 306-357: Bind management-point admission to a canonical projection
covering every adapter-authoritative topology, artifact, and coverage field, or
make those fields immutable outside canonical intake; update management_point.rs
at lines 306-357 accordingly. In
crates/cmtraceopen-parser/src/sccm/server/windows/management_point_tests.rs
lines 591-680, add a mutation that promotes an intake-incomplete artifact to
captured, complete, untruncated, parser/profile-eligible, adds matching
coverage, preserves evidence, and asserts rejection. Add the same
public-entry-point promotion test in
crates/cmtraceopen-parser/tests/sccm_server_management_point.rs lines 45-67.
In
`@crates/cmtraceopen-parser/tests/fixtures/sccm/server/management-point/canonical-intake-policy-scope/manifest.json`:
- Around line 7-11: Update the captured artifact fixture’s rotation metadata to
set rotation.fragmentComplete to true. Preserve the existing topology values and
ensure normalize_topology continues deriving synthetic:site:lab so the fixture
reaches fact parsing.
---
Nitpick comments:
In `@crates/cmtraceopen-parser/src/sccm/server/windows/management_point_tests.rs`:
- Around line 187-189: Update the manifest loading in the test setup to pass the
already-read manifest_json string to load_json instead of constructing another
path and reading manifest.json again.
- Around line 607-617: Update the test around
analyze_management_point_from_server_intake to retain the baseline analysis
result before adding the supplemental IIS artifact, then assert the supplemental
result is identical to that baseline across observations, evidence, and coverage
gaps. Keep the existing skipped, parser-ineligible artifact setup unchanged and
replace the success-only assertion with a direct output comparison.
- Around line 694-697: Update the forged-evidence assertions in
analyze_management_point_from_server_intake and the listed neighboring cases to
use matches! against Err(SccmManagementPointIntakeError::SourceMismatch { .. })
(or each case’s expected named variant) instead of bare is_err(), preserving the
existing failure messages.
In `@crates/cmtraceopen-parser/src/sccm/server/windows/management_point.rs`:
- Around line 377-382: Update the physical_line_end computation near
assessment.evidence to also filter evidence.role == SccmRole::ManagementPoint,
matching the admitted-evidence filter used later in the bundle construction.
Keep the existing artifact_id and line_end filtering and max aggregation
unchanged.
In `@crates/cmtraceopen-parser/tests/sccm_server_management_point.rs`:
- Around line 11-43: Remove the duplicated fixture-loading logic from
canonical_intake and reuse a shared test fixture loader, such as a tests/common
helper or the crate’s test-fixture API, for FIXTURE_ROOT, fixture_directory,
manifest parsing, and payload construction. Preserve this integration test and
its external assess_server_intake call so it continues validating the public
entry point.
🪄 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: 01879773-34b8-4a2a-b81b-526b9c3f2423
⛔ Files ignored due to path filters (1)
crates/cmtraceopen-parser/tests/fixtures/sccm/server/management-point/canonical-intake-policy-scope/evidence/sccm/server/management-point/server-mp-policy/current/MP_GetPolicy.logis excluded by!**/*.log
📒 Files selected for processing (5)
crates/cmtraceopen-parser/src/sccm/server/windows/intake.rscrates/cmtraceopen-parser/src/sccm/server/windows/management_point.rscrates/cmtraceopen-parser/src/sccm/server/windows/management_point_tests.rscrates/cmtraceopen-parser/tests/fixtures/sccm/server/management-point/canonical-intake-policy-scope/manifest.jsoncrates/cmtraceopen-parser/tests/sccm_server_management_point.rs
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
crates/cmtraceopen-parser/src/sccm/server/windows/management_point.rs (2)
41-55: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider marking the public error enum
#[non_exhaustive].
SccmManagementPointIntakeErroris public. Any later variant addition is a breaking change for external callers that match exhaustively. The PR is still a draft, so the cost of adding the attribute now is low.♻️ Proposed change
#[derive(Debug, Clone, PartialEq, Eq, Error)] +#[non_exhaustive] pub enum SccmManagementPointIntakeError {🤖 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/management_point.rs` around lines 41 - 55, Add the #[non_exhaustive] attribute to the public SccmManagementPointIntakeError enum, preserving its existing variants and error definitions.
377-382: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument that
physical_line_endis self-derived from sealed evidence.This bound is the maximum
line_endacross the same evidence that the reducer later validates against it throughevidence_reference_fits_source. On this path the check cannot reject anything, because the bound is derived from the values it must bound.The check is not exploitable here.
adapter_authority_is_intake_boundat Line 306 sealsevidence, and the testcanonical_intake_adapter_rejects_self_attested_line_authorityconfirms that a caller cannot rewriteline_startorline_end.SccmServerArtifactAssessmentcarries no physical line count, so no independent bound exists at this boundary.Add a comment that records this dependency. If the seal is ever relaxed, the reviewer of that change must see that this bound becomes caller-controlled.
📝 Proposed comment
+ // The canonical assessment carries no physical line count, so the + // reducer bound is derived from the sealed evidence itself. This is + // safe only because `adapter_authority_is_intake_bound` already + // rejects any caller rewrite of `line_start` or `line_end`. let physical_line_end = assessment .evidence .iter() .filter(|evidence| evidence.reference.artifact_id == artifact.artifact_id) .filter_map(|evidence| evidence.reference.line_end) .max();🤖 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/management_point.rs` around lines 377 - 382, Add a concise comment immediately above the physical_line_end calculation explaining that it is derived from sealed evidence, and that evidence_reference_fits_source cannot reject values against this self-derived maximum; note that relaxing the intake seal would make this bound caller-controlled and require re-review. Keep the existing calculation unchanged.crates/cmtraceopen-parser/src/sccm/server/windows/intake.rs (1)
1222-1243: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the catalog family mapping instead of duplicating it.
artifact_family_integrity_keyreproducesSccmArtifactFamily::serialized_nameincrates/cmtraceopen-parser/src/sccm/catalog.rs(lines 29-52) arm for arm. Both matches are exhaustive, so the compiler catches a new variant. However, the two copies can drift in their string values without any compile error, and a silent string change alters the integrity byte accounting.Make the catalog mapping
pub(crate)and call it here.♻️ Proposed refactor
In
crates/cmtraceopen-parser/src/sccm/catalog.rs:impl SccmArtifactFamily { - fn serialized_name(&self) -> &str { + pub(crate) fn serialized_name(&self) -> &str {In this file:
-fn artifact_family_integrity_key(family: &SccmArtifactFamily) -> &str { - match family { - SccmArtifactFamily::ClientSetup => "clientSetup", - // ... 17 more arms - SccmArtifactFamily::Unknown(value) => value, - } -} +fn artifact_family_integrity_key(family: &SccmArtifactFamily) -> &str { + family.serialized_name() +}🤖 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/intake.rs` around lines 1222 - 1243, Expose SccmArtifactFamily::serialized_name as pub(crate) in the catalog module, then replace the duplicate match in artifact_family_integrity_key with a call to that shared mapping. Remove the local arm-by-arm string mapping while preserving the existing integrity-key values.
🤖 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/management_point.rs`:
- Around line 297-320: Update the doc comment for
analyze_management_point_from_server_intake to state that it is limited to the
synthetic profile, including the literal synthetic site handle and source
version 5.00.TEST. Clarify that canonical non-synthetic intake bundles are
intentionally rejected with TopologyMismatch, and align the nearby
canonical_intake_site_code documentation with this synthetic-only scope.
- Around line 450-459: Update canonical_fragment_complete so a Captured artifact
with fragment_complete unset and truncated explicitly false returns Some(true),
matching the existing behavior for both fields unset. Preserve the existing
result for fragment_complete values that are present, and add the requested
regression test beside
canonical_intake_adapter_maps_captured_unspecified_fragment_to_complete.
In
`@crates/cmtraceopen-parser/tests/fixtures/sccm/server/management-point/auth-failure/manifest.json`:
- Line 31: Update the auth-failure fixture’s sourceVersion and its paired
expected.json sourceVersionPrefix so they remain compatible, preserving the
required prefix relationship and verifying the fixture expectations together.
In
`@crates/cmtraceopen-parser/tests/fixtures/sccm/server/management-point/healthy-policy/expected.json`:
- Around line 17-19: Verify the extractionProfile admission predicate against
the three 5.00.TEST provenance rows and ensure its sourceVersionPrefix matches
the shortened sourceVersion while retaining the intended profile selection.
Update sourceVersionPrefix only if necessary so the exact version 5.00.TEST is
accepted.
In
`@crates/cmtraceopen-parser/tests/fixtures/sccm/server/management-point/unrelated-client-like-key/expected.json`:
- Around line 17-18: Update the sourceVersion values for the affected synthetic
artifacts in manifest.json and expected.json to 5.00.TEST.0000, matching the
existing sourceVersionPrefix value 5.00.TEST. Preserve the trailing dot in the
prefix and keep all other artifact fields unchanged.
---
Nitpick comments:
In `@crates/cmtraceopen-parser/src/sccm/server/windows/intake.rs`:
- Around line 1222-1243: Expose SccmArtifactFamily::serialized_name as
pub(crate) in the catalog module, then replace the duplicate match in
artifact_family_integrity_key with a call to that shared mapping. Remove the
local arm-by-arm string mapping while preserving the existing integrity-key
values.
In `@crates/cmtraceopen-parser/src/sccm/server/windows/management_point.rs`:
- Around line 41-55: Add the #[non_exhaustive] attribute to the public
SccmManagementPointIntakeError enum, preserving its existing variants and error
definitions.
- Around line 377-382: Add a concise comment immediately above the
physical_line_end calculation explaining that it is derived from sealed
evidence, and that evidence_reference_fits_source cannot reject values against
this self-derived maximum; note that relaxing the intake seal would make this
bound caller-controlled and require re-review. Keep the existing calculation
unchanged.
🪄 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: c5d3ccbc-5e92-410f-a081-4c60ac8e9b41
⛔ Files ignored due to path filters (1)
crates/cmtraceopen-parser/tests/fixtures/sccm/server/management-point/canonical-intake-policy-scope/evidence/sccm/server/management-point/server-mp-policy/current/MP_GetPolicy.logis excluded by!**/*.log
📒 Files selected for processing (23)
crates/cmtraceopen-parser/src/sccm/server/windows/intake.rscrates/cmtraceopen-parser/src/sccm/server/windows/management_point.rscrates/cmtraceopen-parser/src/sccm/server/windows/management_point_tests.rscrates/cmtraceopen-parser/tests/fixtures/sccm/server/management-point/auth-failure/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/server/management-point/auth-failure/manifest.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/server/management-point/canonical-intake-policy-scope/manifest.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/server/management-point/healthy-policy/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/server/management-point/healthy-policy/manifest.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/server/management-point/iis-supplemental/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/server/management-point/iis-supplemental/manifest.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/server/management-point/incomplete/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/server/management-point/incomplete/manifest.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/server/management-point/location-failure/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/server/management-point/location-failure/manifest.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/server/management-point/policy-failure/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/server/management-point/policy-failure/manifest.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/server/management-point/registration-failure/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/server/management-point/registration-failure/manifest.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/server/management-point/rotation-boundary/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/server/management-point/rotation-boundary/manifest.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/server/management-point/unrelated-client-like-key/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/server/management-point/unrelated-client-like-key/manifest.jsoncrates/cmtraceopen-parser/tests/sccm_server_management_point.rs
|
@coderabbitai review Please review exact head |
|
✅ Action performedReview finished.
|
* test(sccm): require canonical MP intake adapter * fix(sccm): reduce management point from server intake * test(sccm): restrict raw management point reducer * test(sccm): reject forged MP intake evidence * fix(sccm): bind MP evidence to canonical intake * test(sccm): expose mutable MP intake metadata * test(sccm): expose MP profile version split * test(sccm): expose MP completeness split * fix(sccm): bind management point intake metadata * test(sccm): bound intake integrity retention * fix(sccm): compact intake integrity binding * test(sccm): preflight nested intake mutations * test(sccm): require single-pass integrity hashing * fix(sccm): preflight sealed intake structure * test(sccm): harden MP intake adapter assertions * test(sccm): expose MP profile prefix drift * test(sccm): align MP fixture profile prefixes * test(sccm): require forward-compatible MP intake errors * fix(sccm): harden MP intake API contracts * test(sccm): harden MP profile fixture contract * test(sccm): scope MP profile fixture validation
Tracking issue: #328. This draft intentionally does not close the issue.
Scope
SccmServerIntakeAssessment.Corrected canonical binding
The first adapter revision bound only normalized evidence. A caller could preserve that evidence while mutating public topology, artifact admission metadata, or coverage. The corrected adapter now seals every adapter-authoritative canonical field with compact, domain-separated identity maps and SHA-256 digests, rejects duplicate identities and cardinality/byte-shape changes, and verifies the seal before canonical cloning or hashing.
The preflight also rejects nested membership or message inflation before retaining large data. Canonical records are hashed once, valid collection ordering remains irrelevant, and exact synthetic fixture semantics are aligned at
5.00.TEST. Captured canonical intake with unspecified truncation/completeness maps to complete only at the private adapter boundary; the manifest contract is not rewritten.The public error is non-exhaustive, artifact-family integrity keys reuse the frozen serializer mapping, and the self-derived physical-line bound is explicitly documented as safe only because canonical intake seals the cited logical-record ranges.
Test-first correction history
ec3473bd: RED for mutable topology/admission metadata.71a30fdf: RED for the unreachable5.00.TESTfixture profile.3b0f073c: RED for canonical captured completeness.92db768b: GREEN binding topology, artifacts, coverage, evidence, version, and completeness.06d6a377: RED proving full serialized retention was a resource regression.06c98cb9: GREEN compact identity/digest binding.f9ca464e: RED for nested membership/string amplification.68e8f052: RED for repeated canonical hashing.9851f448: GREEN preflight of exact structure and single-pass canonical hashing.77a4988f: test-only hardening for exact projection errors and non-vacuous multi-record reordering.6c0e7e6c: RED exposing selected-profile prefix drift.76f43ce8: GREEN aligning all eight selected MP prefixes with exact5.00.TEST.2f5d1177: RED proving external exhaustive matching would freeze the public error enum.29628c9b: GREEN for the non-exhaustive API, synthetic-only docs, sealed-line explanation, and shared family mapping.b8e5e1cd: test-hardening iteration that exposed an over-broad supplemental-version assumption; not accepted as the final contract.28bad9c9: corrected fixture join/profile scope, retaining IIS-specific provenance and role/family exclusion.Fixture and adversarial matrix
IIS.TEST.0000; site-servermpcontroland supplemental IIS cannot enter MP profile selection.Dependencies and limits
codex/parser-family-skeletonat431909709ae3e66bb4c236647af92bd5312bcc90.94195dcca72df0dcdaf33a983b5821710fd369a7. The current three-dot delta owns only Management Point parser/tests/fixtures plus the shared server-intake seal and crate-private artifact-family serializer visibility. Synthetic merge tree6b952c4b0667217f1fd92ccd41605e15dd0a697bis conflict-free with the current target.5.00.TEST; this is not a production ConfigMgr profile claim.Verification at
28bad9c9e6231f1398920a94e00ce11cb188d10fcargo test --locked -p cmtraceopen-parser --test sccm_server_management_point— 3 passed.cargo test --locked -p cmtraceopen-parser --lib— 408 passed.cargo test --locked -p cmtraceopen-parser --test sccm_server_intake— 59 passed.cargo test --locked -p cmtraceopen-parser— every library, integration, and doc target passed.cargo clippy --locked -p cmtraceopen-parser --all-targets -- -D warnings— passed.cargo check --locked -p cmtraceopen-parser --target wasm32-unknown-unknown— passed.git diff --check, and clean-worktree checks — passed.cargo fmt --check --allreports inherited formatting drift outside this exact correction range; all changed Rust files pass scoped Rustfmt.Review state
28bad9c9with no P0-P3 finding after rejecting and correcting the over-broad IIS fixture iteration.77a4988f..28bad9c9and returned zero findings across all 12 corrected files.77a4988fCodeRabbit threads have evidence-backed replies and are resolved; the Captured +truncated:falsesuggestion was rejected because canonical intake does not admit that tuple.Summary by CodeRabbit
New Features
Bug Fixes
Tests