fix(sccm): reject overlapping evidence ranges in the spine - #420
Conversation
`validate_all_evidence_references` keys on (artifact_id, entry_id) and only rejects one identity carrying two ranges. Two references over the same physical lines under different entry ids pass, so one record can be cited twice and `compare_evidence_refs` picks between them arbitrarily. Adds the failing contract across all three citation surfaces plus the serde boundaries, and pins the shapes that must keep validating: adjacent spans, equal spans in different artifacts, and references that assert no extent at all. The `OverlappingEvidenceReference` variant lands inert here so the contract compiles; nothing reads it yet. Refs #418 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Two references over the same physical lines under different entry ids double-count one record and leave `compare_evidence_refs` ranking them on span width alone. Identity equality never saw it, so each reducer had to grow its own guard. The overlap predicate now lives beside the validator every finding already passes through, and `validate_all_evidence_references` sorts each artifact's spans and sweeps them, so the rule holds no matter which reducer, or none, assembled the finding. Semantics are the ones both existing copies already agreed on: inclusive bounds, so equal spans overlap and abutting spans do not; scoped to one artifact; and a reference carrying no bounds asserts no extent and can overlap nothing. An inverted range reads as empty under an inclusive test, so validity is checked first: every reference clears `validate_evidence_reference` before any pair reaches the predicate, and `InvalidEvidenceReference` still wins over the new rejection. The management-point copy is deleted in favour of the spine predicate. The bodies were byte-identical, so admission there is unchanged; its inverted-range handling in particular is preserved, because the reducer compares candidates that have not passed its own safety gate. `finding_rejects_key_or_terminal_refs_that_are_not_cited` built its uncited reference from a helper that pins every span to line 1, so `policy:2-2` claimed line 1 and contradicted its own entry id. The span is now spelled out. Closes #418 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@coderabbitai full review |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe SCCM finding validator now detects overlapping bounded evidence ranges across primary, terminal, and correlation-key references. Management Point validation uses the shared overlap helper. Contract tests cover invalid overlaps, valid ranges, and serialization round trips. ChangesSCCM evidence validation
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
github-actions
Bot
added
bug
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Pull request overview
Extends the SCCM “spine” finding validator to reject overlapping evidence line ranges within the same artifact across distinct (artifact_id, entry_id) identities, preventing double-citation of the same physical records and stabilizing evidence ordering/comparison.
Changes:
- Adds
OverlappingEvidenceReferencetoSccmFindingValidationErrorand enforces anO(n log n)overlap sweep across all three citation surfaces (top-level evidence, terminal evidence, correlation-key evidence). - Centralizes the overlap predicate as
sccm::findings::evidence_references_overlapand reuses it from the management-point reducer (removing a duplicate implementation). - Updates/expands
sccm_spine_contracttests to cover overlap rejection, serde round-trips, and valid disjoint/unbounded cases.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| crates/cmtraceopen-parser/src/sccm/findings.rs | Adds overlap error variant, shared overlap predicate, and spine-level disjoint-span validation across citation surfaces. |
| crates/cmtraceopen-parser/src/sccm/server/windows/management_point.rs | Removes local overlap helper and imports the shared spine predicate to keep semantics unified. |
| crates/cmtraceopen-parser/tests/sccm_spine_contract.rs | Fixes a latent fixture inconsistency and adds comprehensive tests for overlapping/disjoint evidence range behavior. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/cmtraceopen-parser/tests/sccm_spine_contract.rs (1)
1319-1342: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a three-span case to exercise the
widesttracking branch.All six cases use exactly two spans. With two spans, the sweep compares the first against the second and the
widestupdate on line 757 offindings.rsnever affects the outcome. A three-span case reaches that branch: spans1-9,2-3,5-6on one artifact sort to[1-9, 2-3, 5-6], and only the retained1-9span catches5-6. If the sweep tracked the previous span instead of the widest span, that case would pass validation incorrectly and the current suite would not detect the regression.🤖 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_spine_contract.rs` around lines 1319 - 1342, Extend the overlap test cases in evidence_surface_findings to include a three-span artifact scenario with spans 1-9, 2-3, and 5-6, asserting OverlappingEvidenceReference. Ensure the test exercises widest-span retention during the sweep rather than only comparing two spans.
🤖 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/tests/sccm_spine_contract.rs`:
- Around line 1319-1342: Extend the overlap test cases in
evidence_surface_findings to include a three-span artifact scenario with spans
1-9, 2-3, and 5-6, asserting OverlappingEvidenceReference. Ensure the test
exercises widest-span retention during the sweep rather than only comparing two
spans.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c2217dd4-ced4-434b-81bd-7ec34d97faba
📒 Files selected for processing (3)
crates/cmtraceopen-parser/src/sccm/findings.rscrates/cmtraceopen-parser/src/sccm/server/windows/management_point.rscrates/cmtraceopen-parser/tests/sccm_spine_contract.rs
) The CI workflow triggers only on `main`, but the entire SCCM program stacks on `codex/parser-family-skeleton`. Seven open pull requests (#391, #392, #394, #404, #405, #407, #420) therefore run none of the six jobs -- no cargo check/test, no clippy, no MSRV check, no tsc, no Playwright E2E, and no platform builds -- while still reporting as mergeable. GitHub gives no signal for this: a pull request whose base branch is absent from the trigger list is not "pending" or "skipped", it simply has no checks, which is easy to misread as a passing PR with light gating. Add the integration branch to both triggers so lane PRs are gated before they land on it, and so the branch itself is checked after each merge. Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…to codex/pr420-review-repair
|
Review follow-up and CI-base sync — exact head 3615a87 I independently checked the CodeRabbit three-span nitpick before changing the test. The proposed 1-9, 2-3, 5-6 case cannot exercise retained-widest behavior: after sorting, 1-9 and 2-3 overlap, so this validator returns immediately and never inspects 5-6. More generally, because the function exits on the first overlap, the widest update occurs only after a disjoint interval; with start-sorted intervals that current interval is then also the adjacent interval with the furthest end. Adding the proposed case would pass but would not prove the stated branch property, so I did not add a misleading test. I non-force merged the updated integration base solely to activate PR #429 hosted CI. The feature delta remains the same three files. Fresh local gates on the synchronized head: spine 141/141, wasm32 check green, strict parser Clippy green, scoped rustfmt green, and diff check green. @coderabbitai review Please review exact head 3615a87 and re-evaluate the three-span rationale above. A no-finding approval must be attached to this head before merge. |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== checked-out revision =='
git rev-parse HEAD
git status --short
echo
echo '== PR diff summary against merge base =='
base="$(git merge-base HEAD origin/main 2>/dev/null || git merge-base HEAD main 2>/dev/null || true)"
if [ -n "$base" ]; then
git diff --stat "$base"..HEAD -- \
crates/cmtraceopen-parser/src/sccm/findings.rs \
crates/cmtraceopen-parser/src/sccm/server/windows/management_point.rs \
crates/cmtraceopen-parser/tests/sccm_spine_contract.rs
git diff --find-renames --unified=80 "$base"..HEAD -- \
crates/cmtraceopen-parser/src/sccm/findings.rs \
crates/cmtraceopen-parser/src/sccm/server/windows/management_point.rs \
crates/cmtraceopen-parser/tests/sccm_spine_contract.rs
else
echo 'Could not determine merge base with main.'
fi
echo
echo '== source outlines =='
ast-grep outline crates/cmtraceopen-parser/src/sccm/findings.rs --items all --view expanded
ast-grep outline crates/cmtraceopen-parser/src/sccm/server/windows/management_point.rs --items all --view expanded
ast-grep outline crates/cmtraceopen-parser/tests/sccm_spine_contract.rs --items all --view expanded
echo
echo '== overlap definitions and call sites =='
rg -n -C 8 '\bevidence_references_overlap\b|OverlappingEvidenceReference|ConflictingEvidenceReference|InvalidEvidenceReference' \
crates/cmtraceopen-parser/src/sccm/findings.rs \
crates/cmtraceopen-parser/src/sccm/server/windows/management_point.rs \
crates/cmtraceopen-parser/tests/sccm_spine_contract.rsLength of output: 121439 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== revision and remotes =='
git rev-parse HEAD
git remote -v
echo
echo '== changed paths relative to merge base =='
base="$(git merge-base HEAD origin/main 2>/dev/null || git merge-base HEAD main 2>/dev/null)"
git diff --name-status "$base"..HEAD
echo
echo '== findings validation and overlap implementation =='
sed -n '404,780p' crates/cmtraceopen-parser/src/sccm/findings.rs
echo
echo '== management-point usage =='
sed -n '1096,1148p' crates/cmtraceopen-parser/src/sccm/server/windows/management_point.rs
echo
echo '== overlap test helpers and cases =='
sed -n '476,590p' crates/cmtraceopen-parser/tests/sccm_spine_contract.rs
sed -n '1110,1415p' crates/cmtraceopen-parser/tests/sccm_spine_contract.rs
echo
echo '== all helper call sites in Rust source =='
rg -n -C 4 '\bevidence_references_overlap\s*\(' crates/cmtraceopen-parser/src --glob '*.rs'
echo
echo '== focused source diff =='
git diff --unified=25 "$base"..HEAD -- crates/cmtraceopen-parser/src/sccm/findings.rs
echo '== focused management-point diff =='
git diff --unified=25 "$base"..HEAD -- crates/cmtraceopen-parser/src/sccm/server/windows/management_point.rsLength of output: 91054 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== checked-out SHA =='
git rev-parse HEAD
echo
echo '== PR `#420` metadata =='
gh pr view 420 --repo adamgell/cmtraceopen \
--json number,headRefOid,baseRefName,headRefName,changedFiles,files \
--jq '{number, headRefOid, baseRefName, headRefName, changedFiles, files: [.files[].path]}'
echo
echo '== commit metadata =='
git show --no-patch --format='commit=%H%nparents=%P%nsubject=%s' HEAD
echo
echo '== SccmFinding::validate and evidence validation =='
sed -n '404,765p' crates/cmtraceopen-parser/src/sccm/findings.rs
echo
echo '== overlap contract tests =='
sed -n '1262,1418p' crates/cmtraceopen-parser/tests/sccm_spine_contract.rs
echo
echo '== shared helper call sites =='
rg -n -C 3 '\bevidence_references_overlap\s*\(' crates/cmtraceopen-parser/src --glob '*.rs'
echo
echo '== PR file patches, limited to the requested three paths =='
gh api 'repos/adamgell/cmtraceopen/pulls/420/files?per_page=100' \
--jq '.[] | select(.filename == "crates/cmtraceopen-parser/src/sccm/findings.rs" or .filename == "crates/cmtraceopen-parser/src/sccm/server/windows/management_point.rs" or .filename == "crates/cmtraceopen-parser/tests/sccm_spine_contract.rs") | {filename, status, additions, deletions, patch}' \
| head -c 120000
printf '\n'Length of output: 39855 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== precedence-related contract tests =='
rg -n -C 18 'prioritizes|precedence|ConflictingEvidenceReference|InvalidEvidenceReference' \
crates/cmtraceopen-parser/tests/sccm_spine_contract.rs \
| head -n 320
echo
echo '== current conflict-validation block =='
sed -n '649,765p' crates/cmtraceopen-parser/src/sccm/findings.rs
echo
echo '== first-parent conflict-validation block =='
git show HEAD^1:crates/cmtraceopen-parser/src/sccm/findings.rs \
| sed -n '635,710p'
echo
echo '== changed test names and assertions around evidence validation =='
git diff --unified=12 HEAD^1..HEAD -- crates/cmtraceopen-parser/tests/sccm_spine_contract.rs \
| rg -n -C 12 'InvalidEvidenceReference|ConflictingEvidenceReference|OverlappingEvidenceReference|prioritizes|fn finding_' \
| head -n 400Length of output: 21029
I found no review findings. Your three-span rationale is correct. After sorting, I verified the exact PR head and the three-file delta. The implementation preserves artifact scoping, inclusive bounds, unbounded-reference behavior, invalid-reference validation before the overlap sweep, and the Management Point shared-predicate consolidation. The contract coverage includes all three citation surfaces, serialization boundaries, valid disjoint cases, and the overlap shapes. To attach a platform approval, post the explicit top-level command ✏️ Learnings added
✅ 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 6 minutes. |
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/findings.rs:460
SccmFindingValidationErroris a public, non-#[non_exhaustive]enum. Adding the newOverlappingEvidenceReferencevariant is a breaking change for downstream crates that match this error exhaustively; please ensure this is acceptable for the crate’s semver policy (e.g., major bump), or consider making the enum#[non_exhaustive]going forward to avoid repeated breaking changes when new validation errors are added.
MissingRequiredField,
InvalidRole,
InvalidEvidenceReference,
ConflictingEvidenceReference,
OverlappingEvidenceReference,
|
Reviewed Copilots suppressed semver note at exact 3615a87. It is not a blocker for this integration-branch repair: origin/main has no sccm/findings.rs and therefore no released SccmFindingValidationError contract; the new variant is being added within the still-unintegrated SCCM API review sequence, not to an already-published mainline enum. Public LogEntry is unchanged. If the SCCM error type later needs extension after mainline release, non-exhaustive handling can be evaluated as an intentional public-API change rather than silently widening this overlap-fix slice. |
…to codex/pr420-review-repair
|
Exact integration resync checkpoint — Merged current Fresh local verification on this exact head:
Hosted CI run 30724221323 is still in progress. This PR will not merge until the exact-head Copilot review and every required hosted job, including package builds, are green. |
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/findings.rs:461
SccmFindingValidationErroris a public enum in the publishedcmtraceopen-parsercrate. AddingOverlappingEvidenceReferenceis a breaking change for downstream users who exhaustivelymatchon this enum. Consider marking it#[non_exhaustive]to make future additions non-breaking (and ensure the next published version bump reflects the API change).
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub enum SccmFindingValidationError {
MissingRequiredField,
InvalidRole,
InvalidEvidenceReference,
ConflictingEvidenceReference,
OverlappingEvidenceReference,
MissingEvidenceOrCoverageGap,
|
@coderabbitai full review |
✅ Action performedFull review finished. |
Closes #418.
findings.rs::validate_all_evidence_referenceskeyed on(artifact_id, entry_id)and rejected only the same identity carrying a different range. Two references with overlapping line ranges under differententry_ids inside one finding passed validation, so one physical record could be cited twice andcompare_evidence_refswould rank the two on span width alone.Semantics chosen, and why
The predicate now lives in the spine as
evidence_references_overlap:line_start/line_endare inclusive line numbers, so4-6and6-9share line 6 and overlap, while1-2and3-4abut and do not. Equal spans are the degenerate overlap and are rejected.(None, None)on either side is not an overlap. This is what keeps existing citations that omit line numbers valid.validate_evidence_reference(both bounds present or both absent,start > 0,end >= start) inside the same function before any pair reaches the overlap sweep, soInvalidEvidenceReferencestill wins and an inverted range can never be mistaken for an empty span.The deliberate choice not to make the predicate itself fail closed on inverted input is load bearing for the duplication collapse.
management_point.rscalls it fromevidence_identity_is_unique, which compares a candidate against every bundle entry, including entries that never passed the reducer's ownsafe_evidence_referencegate. A fail-closed predicate would let one malformed bundle entry suppress good evidence there. Validity is a separate question from extent, and each caller already answers it.Precedence inside
validate_all_evidence_referencesis now:InvalidEvidenceReference(per reference) ->ConflictingEvidenceReference(one identity, two ranges) ->OverlappingEvidenceReference(two identities, one physical line). The first two are unchanged.The sweep is
O(n log n), not pairwise: references are keyed by identity so each entry id contributes exactly one span, spans are sorted per artifact by start line, and a span that clears the widest span seen so far clears every earlier one.SccmFindingdeserialization has no cap on evidence count, so a quadratic validator on the wire path was not acceptable.Did the two reducer copies agree?
Yes, byte-identical. Verified by extracting both and diffing:
src/sccm/server/windows/management_point.rslines 1130-1144 (merged, SCCM Server: diagnose management-point client transactions #328)src/sccm/client/policy.rslines 755-769 onorigin/codex/sccm-321-policy-analysis(in flight, feat(sccm): analyze client policy transactions #391)Both produced the same text for artifact scoping, inclusive-bounds comparison, and missing-bound handling. No edge case disagreed, so there was no semantics dispute to resolve, and the spine predicate is that text with the reasoning written down.
Duplication collapse
The management-point copy is deleted; the reducer imports the spine predicate. The removed body is byte-identical to the spine one, so admission there is unchanged, and
sccm_server_management_pointpasses unmodified (26/26).policy.rsdoes not exist oncodex/parser-family-skeletonyet, so its copy cannot be collapsed here. #391 should dropevidence_references_overlapfrompolicy.rswhen it rebases and importcrate::sccm::findings::evidence_references_overlapinstead.quarantine_overlapping_evidencestays where it is: it is a reducer policy (quarantine every participant rather than reject the bundle), not the predicate.Shipped-fixture audit
Every
expected.jsonin the tree was scanned for two references with distinctentryIdand overlapping ranges inside one artifact, across all three citation surfaces (evidence,terminalEvidence[].reference,correlationKeys[].evidence). The same scan was run against every in-flight SCCM lane branch, since this rejection lands under work already in review.codex/parser-family-skeleton(065b8cb)codex/sccm-321-policy-analysis(#391)codex/sccm-320-health-analysis(#392)codex/sccm-319-pure-intake(#394)codex/sccm-spine-findings-hardening(#404)codex/sccm-327-site-core-reducer(#405)codex/sccm-322-deployment-reducer(#407)No shipped fixture carries the shape. Reducer output is covered by the fixture contract suites: they run each reducer over its corpus and diff against
expected.json, and management point in particular doesbuilder.build().ok()?, so a newly rejected finding would silently vanish and fail the comparison. All of them stay green, so no reducer emits an overlapping citation today either.One test fixture did carry the shape, and it is reported rather than papered over.
finding_rejects_key_or_terminal_refs_that_are_not_citedbuilt its uncited reference withfinding_evidence_ref("client-policy-agent", "policy:2-2"), and that helper pins every span to line 1 regardless of the entry id. Sopolicy:2-2claimed line 1 alongsidepolicy:1-1, which is exactly the defect: two entry ids over one physical line, with entry ids that advertise ranges contradicting their own bounds. The fix spells the span out as2-2to match the entry id it advertises. The test's subject, that an uncited reference is rejected, is unchanged and still assertsCorrelationKeyEvidenceNotCited/TerminalEvidenceNotCited. This was a latent inconsistency in the fixture, not a semantics disagreement.RED / GREEN
19addcdbadded the contract plus the inertOverlappingEvidenceReferencevariant so it compiles, and failed with the spine accepting the shape:called Result::unwrap_err() on an Ok value: SccmFinding { ... evidence: [SccmEvidenceRef { artifact_id: "artifact-a", entry_id: "entry-a", line_start: Some(4), line_end: Some(6) }, SccmEvidenceRef { artifact_id: "artifact-a", entry_id: "entry-b", line_start: Some(4), line_end: Some(6) }] }. 139 passed, 2 failed.8b487a35wired the predicate into the validator and collapsed the management-point copy. 141 passed, 0 failed.New coverage goes through the real
SccmFinding::validatepath, not a unit helper: builder, directvalidate(),serdedeserialization, and the validating serializer. Six overlap shapes (identical, shared start line, shared end line, contained, containing, straddling) are each exercised on all three citation surfaces, and the shapes that must keep validating are pinned too: adjacent spans, equal spans in different artifacts, one bounded plus one unbounded, and both unbounded.Verification
All from the worktree, all clean.
cargo test --locked -p cmtraceopen-parser --test sccm_spine_contract: 141 passed, 0 failed (138 before).cargo test --locked -p cmtraceopen-parser: 1039 passed, 0 failed across 21 binaries. Baseline on 065b8cb was 1036; the delta is exactly the 3 new spine tests, every other binary count is identical.cargo test --locked --workspace: 1793 passed, 0 failed across 36 binaries.cargo clippy --locked -p cmtraceopen-parser --all-targets -- -D warnings: clean.cargo check --locked -p cmtraceopen-parser --target wasm32-unknown-unknown: clean.npx tsc --noEmit: clean.git diff --check 065b8cb7 HEAD: clean.rustfmt --checkon all three changed files: clean.Every SCCM contract suite in the tree, run individually, all green:
sccm_client_deployment_fixture_contractsccm_client_health_fixture_contractsccm_client_intake_fixture_contractsccm_client_inventory_compliance_metering_fixture_contractsccm_client_management_fixture_contractsccm_client_task_sequence_fixture_contractsccm_client_updates_fixture_contractsccm_correlation_contractsccm_server_advanced_roles_catalogsccm_server_distribution_point_fixture_contractsccm_server_hierarchy_and_replication_fixture_contractsccm_server_intakesccm_server_intake_fixture_contractsccm_server_management_pointsccm_server_provider_and_admin_service_fixture_contractsccm_server_software_update_point_fixture_contractsccm_site_core_fixture_contractsccm_spine_contractReviewer attention
SccmFindingValidationErrorgains a variant. It is#[derive(Debug, Clone, Copy, PartialEq, Eq)]with no#[non_exhaustive], and nothing in the tree matches on it exhaustively, but any lane holding a match should be checked on rebase.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests