feat(sccm): analyze Management Point evidence - #393
Conversation
📝 WalkthroughWalkthroughAdds a public SCCM Management Point analyzer. It validates evidence and provenance, reduces workflow phases into transactions, classifies outcomes, emits findings and coverage data, and verifies deterministic serialized results with integration fixtures. ChangesSCCM Management Point Analysis
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
|
GREEN implementation is now pushed at exact head Focused reducer: 13/13 passed. Intake: 1/1. Spine: 136/136. Full parser suite: passed. Strict Clippy: passed. Rust 1.88 wasm32 check: passed. Scoped rustfmt, 18 fixture JSON validations, and TDD evidence is preserved in three RED commits ( Repository-wide formatting still reports 17 inherited non-#328 files; all #328-owned files pass scoped rustfmt. No live Windows acceptance is claimed. Please review the exact pushed head; merge remains gated on substantive CodeRabbit and a different independent exact-head review. |
|
@coderabbitai review Please review exact head |
|
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
crates/cmtraceopen-parser/src/sccm/server/windows/management_point.rs (2)
1010-1020: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueUse
continueinstead of?for a malformed braced value.Line 1011 returns
Nonefromtoken_valuewhen the first braced occurrence has no closing}. A later well-formed occurrence of the same label is then never inspected. Every other rejection path in this loop usescontinue.The current behavior fails closed, so this is a consistency fix, not a defect.
♻️ Proposed consistency fix
if let Some(braced) = remainder.strip_prefix('{') { - let end = braced.find('}')?; + let Some(end) = braced.find('}') else { + continue; + }; let suffix = &braced[end + 1..];🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/cmtraceopen-parser/src/sccm/server/windows/management_point.rs` around lines 1010 - 1020, Update the braced-value parsing in token_value to treat a missing closing brace as a rejected occurrence and continue scanning subsequent matches, rather than returning None immediately. Preserve the existing boundary validation and successful return behavior for well-formed braced values.
783-783: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winA rejected finding is dropped without a trace, and its coverage gap stays.
builder.build().ok()?discards theSccmFindingValidationError. The caller then still pushes the coverage gap and the artifact requests into the analysis, asreduce_transactiondoes at Lines 340-343. The output can therefore contain a gap and a request with no finding that explains them.build_source_local_findinghas the same pattern at Line 1441.Propagate the error, or record a deterministic diagnostic, so a contract regression is observable instead of silent.
🤖 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` at line 783, Update the finding construction in the current builder flow and in build_source_local_finding to avoid discarding SccmFindingValidationError via build().ok()?; propagate the error through the existing result path or emit a deterministic diagnostic before returning. Ensure rejected findings cannot silently leave coverage gaps or artifact requests without an observable validation failure.crates/cmtraceopen-parser/tests/sccm_server_management_point.rs (1)
196-218: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCompare the full
nextArtifactslist, not only the first entry.Line 210-214 reduces
nextArtifactsto its firstlogicalArtifactId.source_local_projectiondoes the same at Lines 331-339. A regression that appends a second, unrelated artifact request to a transaction or an observation passes these assertions.Project the complete ordered list of logical ids so the request contract is fully pinned.
♻️ Proposed projection change
- "nextArtifactLogicalId": transaction["nextArtifacts"] - .as_array() - .and_then(|requests| requests.first()) - .map(|request| request["logicalArtifactId"].clone()) - .unwrap_or(Value::Null), + "nextArtifactLogicalIds": transaction["nextArtifacts"] + .as_array() + .map(|requests| { + requests + .iter() + .map(|request| request["logicalArtifactId"].clone()) + .collect::<Vec<_>>() + }) + .unwrap_or_default(),Apply the matching change to
expected_transaction_projectionand update the fixture expectations to declare the full list.🤖 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 196 - 218, Update actual_transaction_projection and source_local_projection to project the complete ordered nextArtifacts logicalArtifactId list rather than only the first entry; apply the same change to expected_transaction_projection and revise fixture expectations to include the full lists.
🤖 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 791-836: Update build_counterpart_fact to restrict candidate facts
to the transaction’s decided phase, preserving the existing policy and timestamp
eligibility checks. When transaction.state is Failed, additionally require the
selected fact to represent a terminal failure outcome before constructing
SccmManagementPointCounterpartReadyFact, so evidence and terminal_evidence
cannot come from a succeeded record.
---
Nitpick comments:
In `@crates/cmtraceopen-parser/src/sccm/server/windows/management_point.rs`:
- Around line 1010-1020: Update the braced-value parsing in token_value to treat
a missing closing brace as a rejected occurrence and continue scanning
subsequent matches, rather than returning None immediately. Preserve the
existing boundary validation and successful return behavior for well-formed
braced values.
- Line 783: Update the finding construction in the current builder flow and in
build_source_local_finding to avoid discarding SccmFindingValidationError via
build().ok()?; propagate the error through the existing result path or emit a
deterministic diagnostic before returning. Ensure rejected findings cannot
silently leave coverage gaps or artifact requests without an observable
validation failure.
In `@crates/cmtraceopen-parser/tests/sccm_server_management_point.rs`:
- Around line 196-218: Update actual_transaction_projection and
source_local_projection to project the complete ordered nextArtifacts
logicalArtifactId list rather than only the first entry; apply the same change
to expected_transaction_projection and revise fixture expectations to include
the full lists.
🪄 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: 3649d500-3b88-4daf-8ddb-008c230d64e1
📒 Files selected for processing (5)
crates/cmtraceopen-parser/src/sccm/mod.rscrates/cmtraceopen-parser/src/sccm/server/mod.rscrates/cmtraceopen-parser/src/sccm/server/windows/management_point.rscrates/cmtraceopen-parser/src/sccm/server/windows/mod.rscrates/cmtraceopen-parser/tests/sccm_server_management_point.rs
|
CodeRabbit blocker reproduced and corrected at exact head TDD:
Aggregate exact-head gates: full parser passed; strict Clippy passed; Rust 1.88 wasm32 passed; scoped rustfmt, all 18 MP fixture JSON documents, diff check, and clean status passed. The existing repository-wide fmt exception remains 17 inherited non-#328 files. The CodeRabbit thread was answered and resolved. PR remains draft/unmerged pending a fresh substantive exact-head CodeRabbit result and a different independent exact-head PASS. No live Windows acceptance is claimed. |
|
@coderabbitai review Please perform a fresh substantive review of exact head |
|
✅ 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 48 minutes. |
adamgell
left a comment
adamgell
left a comment
There was a problem hiding this comment.
Independent exact-head review of ba957f1: BLOCKED.
Baseline focused target is green at 14/14. I then added four temporary review-only adversarial regressions, ran them together, observed 0/4, and removed them so the detached review worktree is clean.
P1 — duplicate exact-key labels are accepted. token_value at management_point.rs:1003-1038 returns the first valid occurrence and never rejects a second conflicting occurrence. Appending RequestId={28999999-9999-9999-9999-999999999999} to every healthy record still produced a High succeeded transaction. Exact profile keys must be unambiguous; preserve a permanent conflicting-label regression for RequestId and the other correlation-bearing labels/results.
P1 — later deferred evidence is ignored. resolve_phase at lines 631-684 considers Deferred only when there is no Success. Swapping the healthy Respond records so success is earlier and deferred is later still completed the transaction as High succeeded. Reduce the latest same-phase disposition, with same-instant conflicts conservative, so only later same-key success can recover deferred work.
P1 — event names are prefix matches, not exact profile tokens. parse_phase_outcome at lines 897-954 accepts Record outcome succeededness because starts_with has no boundary check; the mutated bundle still completed High succeeded. Require a delimiter/end boundary after every admitted event marker and keep success/failure/deferred suffix mutations as regressions.
P1 — one physical evidence identity can authorize conflicting exact keys. The evidence loop at lines 290-325 has no uniqueness/conflict gate for evidence_id/reference identity. Cloning the complete healthy evidence set, changing request/policy/client keys while retaining every original evidence reference, produced two High succeeded transactions from the same physical lines. Reject or deterministically quarantine conflicting reuse before facts are grouped; no reused identity may authorize either exact-key transaction.
The ba957f1 terminal-failure counterpart correction itself is sound in the reviewed mutation. CodeRabbit final-head approval was fair-usage limited and is not counted as a substantive exact-head review. Its full-nextArtifacts assertion gap remains actionable test hardening; the build().ok()? silent-drop observation should be resolved or shown unreachable without weakening fail-closed behavior.
Keep PR393 draft/open. Add the four focused RED tests first, implement the smallest fixes, rerun focused/full parser/strict Clippy/Rust 1.88 wasm/scoped formatting/JSON/diff gates, request fresh substantive CodeRabbit, then obtain a different exact-head review.
|
Independent exact-head review 4828957761 reproduced four P1 fail-closed defects at ba957f1: conflicting duplicate key labels, later deferred evidence overridden by earlier success, non-delimited event prefixes, and conflicting exact keys reusing the same physical evidence identity. Temporary probes were 0/4 while the committed focused target remained 14/14. Correction is claimed in fresh isolated worktree /Users/Adam.Gell/repo/cmtraceopen/.worktrees/sccm-328-ba95-round2 on branch codex/sccm-328-ba95-round2 at exact base ba957f1. I will preserve the four probes as the next test-only RED commit, then implement the smallest GREEN. The PR remains draft/open; the fair-usage-limited CodeRabbit approval is not counted. |
|
Correction pushed at exact head TDD:
The reducer now rejects duplicate/malformed exact-profile tokens; uses the latest same-phase disposition with equal-instant conflicts conservative; requires exact event-marker boundaries; and quarantines reused evidence IDs, entry IDs, or physical ranges before fact grouping. The frozen projections now compare every ordered Exact-head verification:
Review-note disposition:
Local CodeRabbit was invoked on the exact correction range but is currently rate-limited for four minutes; it is not counted as approval. PR remains draft/open pending a substantive exact-head CodeRabbit result and a different independent exact-head PASS. No live Windows acceptance is claimed. @coderabbitai review Please perform a fresh substantive review of exact head |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== checked-out revision =='
git rev-parse HEAD
git status --short
echo '== PR metadata =='
gh pr view 393 --repo adamgell/cmtraceopen --json number,headRefName,headRefOid,baseRefName,title,isDraft
echo '== changed files against merge base =='
base="$(git merge-base HEAD origin/main 2>/dev/null || git merge-base HEAD main)"
git diff --name-status "$base"...HEAD
echo '== management-point structural outline =='
ast-grep outline crates/cmtraceopen-parser/src/sccm/server/windows/management_point.rs --items all --type function,struct,enum
echo '== management-point test outline =='
ast-grep outline crates/cmtraceopen-parser/tests/sccm_server_management_point.rs --items all --type functionLength of output: 47658 🏁 Script executed: #!/bin/bash
set -euo pipefail
file='crates/cmtraceopen-parser/src/sccm/server/windows/management_point.rs'
test_file='crates/cmtraceopen-parser/tests/sccm_server_management_point.rs'
echo '== recent commits affecting the reducer =='
git log --oneline --decorate -12 -- "$file" "$test_file"
echo '== correction diff from first parent =='
git diff HEAD^..HEAD -- "$file" "$test_file"
echo '== reducer intake, grouping, and transaction reduction =='
sed -n '273,616p' "$file"
echo '== phase decision and counterpart handoff =='
sed -n '617,810p' "$file"
echo '== parsing, exact marker, and token validation =='
sed -n '811,1087p' "$file"
echo '== evidence identity validation and output normalization =='
sed -n '1088,1182p' "$file"
sed -n '1588,1680p' "$file"
echo '== adversarial correction tests =='
sed -n '1000,1175p' "$test_file"Length of output: 63137 🏁 Script executed: #!/bin/bash
set -euo pipefail
file='crates/cmtraceopen-parser/src/sccm/server/windows/management_point.rs'
test_file='crates/cmtraceopen-parser/tests/sccm_server_management_point.rs'
echo '== remaining reducer helper paths =='
sed -n '1180,1587p' "$file"
echo '== fixture contract assertions and counterpart tests =='
sed -n '229,500p' "$test_file"
sed -n '530,715p' "$test_file"
echo '== transaction and counterpart public-model validations =='
ast-grep outline crates/cmtraceopen-parser/src/sccm/models.rs --items all --type function,struct,enum --match 'Finding|Evidence|Key|Counterpart|ManagementPoint'
rg -n -C 4 'struct SccmEvidence|struct SccmEvidenceRef|struct SccmFindingBuilder|fn build\(' crates/cmtraceopen-parser/src/sccm
echo '== management-point API consumers =='
rg -n -C 3 '\banalyze_management_point\s*\(|SccmManagementPointCounterpartReadyFact|cross_side_correlation_performed' crates --glob '*.rs'Length of output: 49622 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== exact reducer lines for evidence-range admission =='
nl -ba crates/cmtraceopen-parser/src/sccm/server/windows/management_point.rs | sed -n '292,325p;1088,1145p'
echo '== test bundle construction and evidence references =='
nl -ba crates/cmtraceopen-parser/tests/sccm_server_management_point.rs | sed -n '110,176p'
echo '== all constructors and validators for SccmManagementPointBundle and SccmEvidence =='
rg -n -C 4 'SccmManagementPointBundle\s*\{|SccmEvidence\s*\{|line_start:|line_end:' crates/cmtraceopen-parser --glob '*.rs'
echo '== overlap validation in SCCM evidence and finding modules =='
rg -n -i -C 4 'overlap|non.?overlap|line.*range|range.*line|line_start.*line_end' crates/cmtraceopen-parser/src/sccm crates/cmtraceopen-parser/tests --glob '*.rs'Length of output: 1205 🏁 Script executed: #!/bin/bash
set -euo pipefail
numbered_slice() {
local path="$1"
local start="$2"
local end="$3"
awk -v start="$start" -v end="$end" 'NR >= start && NR <= end { printf "%6d %s\n", NR, $0 }' "$path"
}
echo '== exact reducer lines for evidence-range admission =='
numbered_slice crates/cmtraceopen-parser/src/sccm/server/windows/management_point.rs 292 325
numbered_slice crates/cmtraceopen-parser/src/sccm/server/windows/management_point.rs 1088 1145
echo '== test bundle construction and evidence references =='
numbered_slice crates/cmtraceopen-parser/tests/sccm_server_management_point.rs 110 176
echo '== all constructors and validators for management-point bundles and evidence =='
rg -n -C 4 'SccmManagementPointBundle\s*\{|SccmEvidence\s*\{|line_start:|line_end:' crates/cmtraceopen-parser --glob '*.rs'
echo '== overlap validation in SCCM evidence and finding modules =='
rg -n -i -C 4 'overlap|non.?overlap|line.*range|range.*line|line_start.*line_end' crates/cmtraceopen-parser/src/sccm crates/cmtraceopen-parser/tests --glob '*.rs' || trueLength of output: 42119
Found one evidence-boundary issue.
You are interacting with an AI system. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
expected='0d1ccefdfeb2d2021e875ffe80a8cae4af99ccec'
actual="$(git rev-parse HEAD)"
printf 'HEAD=%s\nEXPECTED=%s\n' "$actual" "$expected"
if [ "$actual" != "$expected" ]; then
echo 'ERROR: checked-out revision does not match the requested exact head.' >&2
exit 2
fi
git diff --unified=80 "${expected}^" "$expected" -- \
crates/cmtraceopen-parser/src/sccm/server/windows/management_point.rs \
crates/cmtraceopen-parser/tests/sccm_server_management_point.rs
ast-grep outline crates/cmtraceopen-parser/src/sccm/server/windows/management_point.rs --items all
ast-grep outline crates/cmtraceopen-parser/tests/sccm_server_management_point.rs --items all
printf '\n--- Correction-related source matches ---\n'
rg -n -C 8 -e 'build\(\)\.ok\(\)\?' -e 'nextArtifacts' -e 'next_artifacts' \
-e 'physical' -e 'evidence_id' -e 'entry_id' -e 'marker' -e 'disposition' \
-e 'exact' -e 'duplicate|Duplicate|malformed|Malformed|conflict|Conflict' \
crates/cmtraceopen-parser/src/sccm/server/windows/management_point.rs \
crates/cmtraceopen-parser/tests/sccm_server_management_point.rsLength of output: 191942 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- Transaction reduction and output normalization ---'
sed -n '390,555p' crates/cmtraceopen-parser/src/sccm/server/windows/management_point.rs
sed -n '757,811p' crates/cmtraceopen-parser/src/sccm/server/windows/management_point.rs
sed -n '1580,1665p' crates/cmtraceopen-parser/src/sccm/server/windows/management_point.rs
printf '%s\n' '--- Shared finding builder definitions and build callers ---'
rg -n -C 12 -P '\bstruct\s+SccmFindingBuilder\b|\bimpl\s+SccmFindingBuilder\b|\bfn\s+build\s*\(' crates/cmtraceopen-parser/src
rg -n -C 8 -P '\.build\(\)\.ok\(\)\?' crates/cmtraceopen-parser/src
printf '%s\n' '--- Counterpart and no-correlation tests ---'
sed -n '540,640p' crates/cmtraceopen-parser/tests/sccm_server_management_point.rs
sed -n '430,480p' crates/cmtraceopen-parser/tests/sccm_server_management_point.rsLength of output: 27615 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- SccmFindingBuilder validation rules ---'
sed -n '450,720p' crates/cmtraceopen-parser/src/sccm/findings.rs
printf '%s\n' '--- Management Point shared request and gap mappings ---'
sed -n '1410,1495p' crates/cmtraceopen-parser/src/sccm/server/windows/management_point.rs
printf '%s\n' '--- Complete four correction tests ---'
sed -n '1020,1185p' crates/cmtraceopen-parser/tests/sccm_server_management_point.rs
printf '%s\n' '--- All builder validation error variants and request limits ---'
rg -n -C 4 -e 'MAX_SCCM_NEXT_ARTIFACT_REQUESTS' -e 'enum SccmFindingValidationError' \
-e 'return Err\(SccmFindingValidationError' crates/cmtraceopen-parser/src/sccm/findings.rsLength of output: 25166
I found no blocking issue in the requested correction. The four ambiguity changes are fail-closed:
The failed counterpart handoff remains bounded to the reduced phase. It requires terminal failed evidence for a failed transaction. A later successful fact cannot replace that citation. The The output ordering remains deterministic. The reducer sorts transactions, observations, findings, coverage gaps, counterpart facts, and aggregate artifact requests. The code keeps cross-side correlation disabled. I did not execute the test suite in this sandbox. The sandbox does not provide compiler or interpreter execution. You are interacting with an AI system. |
|
Independent round-two exact-head review active in fresh detached worktree |
adamgell
left a comment
adamgell
left a comment
There was a problem hiding this comment.
Independent round-two exact-head review: BLOCK
Reviewed the full a8a367c..0d1ccef range in a fresh detached, read-only worktree. I did not author #328 and made no edits or pushes.
Committed gates
- Focused Management Point target: 18/18 pass.
- Server intake: 1/1 pass; current site-core fixture target: 5/5 pass; spine is green in the full run.
- Full cmtraceopen-parser suite: pass.
- Strict parser Clippy: pass.
- Rust 1.88 wasm32 check: pass.
- All 18 Management Point JSON files: pass.
- All five PR-owned Rust files pass Rust 1.88 rustfmt; git diff --check passes; detached worktree is clean.
- Workspace cargo fmt remains red only on inherited paths outside this five-file PR range.
- The plan command names sccm_server_site_core, which is not a current test target; the current sccm_site_core_fixture_contract target passed 5/5.
Independent adversarial evidence
A disposable external harness imported all 18 committed tests and added eight probes. Command: cargo test --test probe -- --nocapture. Result: 21 passed and 5 failed against this exact SHA.
- Overlapping physical ranges are accepted. Changing three policy references to overlapping ranges 2-3, 3-4, and the existing 4-4, all with unique evidence and entry IDs, still emits one deterministic high-confidence complete success. This independently reproduces the hosted CodeRabbit High finding.
- Malformed compound labels are treated as exact. Prefixing every required label with Not- preserves high success for RequestId, PolicyId, ClientHandle, SiteCode, and MPHandle; Not-Result still proves a high confirmed terminal failure.
- A successful counterpart handoff can cite terminal-failure evidence. An earlier policy-bound RecordOutcome terminal failure followed by a later recovered success without PolicyId yields a high successful transaction whose successful counterpart cites the earlier failed line 2.
- A success marker with Result=0x80004005 still emits a high successful transaction and successful counterpart fact. The explicit nonzero result is contradictory terminal evidence and cannot be ignored.
- Transaction evidence is not record-exact. When policy line 2 is changed to an unprofiled/rejected narrative, the transaction remains high success and merge_references coalesces accepted lines 1, 3, and 4 into a synthetic 1-4 citation, thereby citing the rejected line as supporting evidence.
Positive adversarial checks passed: conflicting same-instant outcomes are contradictory; malformed unclosed-brace material cannot recover through a later valid occurrence; all non-success transactions and coverage gaps in the frozen corpus have observable findings; the committed duplicate-ID, exact-range reuse, topology, version, invalid-time, coverage, privacy, deterministic-reorder, latest-state, marker-suffix, and failed-terminal-handoff checks remain green.
Required before re-review
- Reject intersecting physical intervals within one artifact, not only byte-for-byte equal ranges. Quarantine all participants before grouping and preserve input-order-independent output.
- Use the frozen shared exact-key token boundaries or shared profile extractor. A hyphen is not a valid boundary for these labels; keep all six Not- label regressions plus existing duplicate/malformed cases.
- Build counterpart facts from the actual decisive phase fact. A Success handoff must cite succeeded evidence that carries the exact policy key; a Failed handoff must cite the decided terminal failure. Never copy transaction state/classification onto an opposite-outcome fact.
- Reconcile explicit result signals with marker outcome. Nonzero Result on a success marker is contradictory or unusable, never high success.
- Preserve accepted logical-record references rather than min/max coalescing across gaps. No rejected, malformed, overlapping, or unrelated physical line may be absorbed into a high-confidence citation.
- Commit all five failing probes and the three positive guards with focused RED-to-GREEN evidence, then rerun the aggregate gates and both substantive reviewers.
The current builder-observability probe did not reproduce a silent build drop: every frozen non-success and gap remained represented by a finding. Keep that invariant as a permanent test because build().ok()? still makes future contract rejection non-observable, but it is not a separate reproduced blocker in this review.
CodeRabbit state
Hosted CodeRabbit inspected this exact SHA and independently reported the overlapping-range defect as High at #393 (comment). Its later no-block text did not address or invalidate that finding and did not execute tests. Local CodeRabbit 0.7.1 exact-range review is currently rate-limited for 25 minutes and is not approval.
Keep PR #393 draft/open. #333 policy-to-MP implementation remains blocked until repaired #321 and #328 counterpart facts have exact-head independent and substantive CodeRabbit approval. No live Windows acceptance is claimed.
|
@coderabbitai review Please perform a substantive exact-head review of 187c529. This supersedes d1c3447. |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
expected='187c529b972e25d1c0a87e74055e546097b3f440'
actual="$(git rev-parse HEAD)"
printf 'HEAD=%s\nEXPECTED=%s\n' "$actual" "$expected"
if [ "$actual" != "$expected" ]; then
echo 'ERROR: checked-out revision does not match the requested exact head.' >&2
exit 2
fi
echo '== PR head =='
gh pr view 393 --repo adamgell/cmtraceopen \
--json headRefOid,isDraft,title \
--jq '{headRefOid, isDraft, title}'
src='crates/cmtraceopen-parser/src/sccm/server/windows/management_point.rs'
tests='crates/cmtraceopen-parser/tests/sccm_server_management_point.rs'
echo '== correction diff =='
git diff --unified=60 "${expected}^..${expected}" -- "$src" "$tests"
echo '== reducer structure =='
ast-grep outline "$src" --items all --type function,struct,enum
echo '== exact counterpart paths =='
rg -n -C 8 \
'build_counterpart_fact|build_transaction_key|policy_id|PolicyId|decisive|terminal|outcome' \
"$src"
echo '== exact correction regression =='
rg -n -C 10 \
'successful_counterpart_handoff_requires_the_decisive_fact_to_prove_the_policy_key|decisive.*policy|PolicyId' \
"$tests"
echo '== evidence and output reference paths =='
rg -n -C 6 \
'evidence_identity_is_unique|merge_references|references|line_start|line_end|rejected_references' \
"$src"Length of output: 50830 🏁 Script executed: #!/bin/bash
set -euo pipefail
src='crates/cmtraceopen-parser/src/sccm/server/windows/management_point.rs'
tests='crates/cmtraceopen-parser/tests/sccm_server_management_point.rs'
numbered() {
local path="$1" start="$2" end="$3"
awk -v start="$start" -v end="$end" \
'NR >= start && NR <= end { printf "%6d %s\n", NR, $0 }' "$path"
}
echo '== reduction decision propagation =='
numbered "$src" 402 555
numbered "$src" 618 678
numbered "$src" 758 861
echo '== marker, label, and Result validation =='
numbered "$src" 937 1086
echo '== rejected-evidence observations and deterministic normalization =='
numbered "$src" 1261 1340
numbered "$src" 1590 1685
echo '== all five adverse-regression tests =='
numbered "$tests" 1130 1345
echo '== test registration and fixture assertions around counterpart output =='
rg -n -C 6 \
'counterpartReadyFacts|counterpart_ready_facts|assert.*determin|analysis_value|load_bundle' \
"$tests"Length of output: 66280
I found no blocking issue in the counterpart-handoff correction.
The new regression removes The inspected reducer also retains the prior protections:
I did not execute tests in this sandbox. The sandbox does not provide compiler or interpreter execution. ✅ Action performedReview finished.
|
|
Independent review assignment withdrawn without verdict: PR head advanced from the claimed |
adamgell
left a comment
adamgell
left a comment
There was a problem hiding this comment.
Independent round-three exact-head review: PASS
Reviewed exact a8a367c661cd17bd1e73b87724c143ec8565ffc7..187c529b972e25d1c0a87e74055e546097b3f440 in fresh detached worktree /Users/Adam.Gell/repo/cmtraceopen/.worktrees/review-328-187c-round2. Final local, remote-branch, and PR-head guards all matched 187c529b972e25d1c0a87e74055e546097b3f440. I did not edit or push.
Adversarial verification
- Overlap quarantine: the committed overlapping-range regression passes. A separate containment probe expanded one policy record over lines 1–4; every intersecting participant was quarantined, no High success survived, and reversing bundle order produced byte-equivalent JSON.
- Exact labels: all committed
Not-RequestId,Not-PolicyId,Not-ClientHandle,Not-SiteCode,Not-MPHandle, andNot-Resultguards pass. An independent hyphenated decisive-policy probe produced neither High success nor a counterpart handoff. - Outcome agreement: successful and failed counterpart facts were independently resolved back to their exact input records. Success citations contain succeeded evidence and no terminal reference; failure citations contain terminal-failure evidence and use that same reference as terminal evidence.
- Result coherence: nonzero
Resulton a success marker produces neither High success nor a counterpart; zero-result success remains admitted and zero-result failure cannot become confirmed failure. - Record-exact citations: every healthy transaction reference matched one exact accepted input
artifactId/entryId/line range; no fabricated min/max span remained. The rejected-line regression stays green and deterministic. - Hosted CodeRabbit P1: every emitted counterpart citation was independently checked to contain the exact
PolicyIdexported in its key. The decisive-fact-without-policy regression emits no counterpart. Hosted CodeRabbit also reviewed this exact SHA and reported no blocker: #393 (comment).
External disposable harness: 27/27 pass (23 committed tests plus 4 independent probes). The harness lived outside the repository; the detached review worktree remains clean.
Gates
cargo test --locked -p cmtraceopen-parser --test sccm_server_management_point— PASS 23/23cargo test --locked -p cmtraceopen-parser— PASScargo clippy --locked -p cmtraceopen-parser --all-targets -- -D warnings— PASScargo check --locked -p cmtraceopen-parser --target wasm32-unknown-unknown— PASS- all 18 Management Point fixture JSON files — PASS
- privacy regression, all five PR-owned Rust files under Rustfmt, PR-range
git diff --check, and clean detached status — PASS - workspace-wide Rustfmt still has the inherited unrelated ESP/Tauri baseline; no unrelated formatting was pulled into #328.
This PASS is limited to the exact #328 parser/source contract at 187c529b. Keep PR #393 draft/open: #335/native Windows acceptance remains unproven, and #333 policy-to-MP correlation remains gated on stable reviewed #321 + #328 source facts and its own adversarial correlation implementation. No cross-side causal or live-Windows acceptance claim is approved.
|
@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 56 minutes. |
The merged skeleton (#396) models mpcontrol.log as produced by the site-server MP control workflow about the Management Point, not by the MP role itself. Freeze that contract in the reducer-source test, flip the iis-supplemental fixture to the site-server producer role, and add failing tests: a site-server mpcontrol capture must not shape MP-produced policy coverage states, and an mpcontrol source claiming MP production must fail closed as rejected evidence. Refs #328
Key the subject-scoped mpcontrol supplemental check on the shared catalog identity (site-server producer role, ManagementPoint family) instead of the hardcoded producer string, and exclude supplemental sources from MP-produced coverage states so a site-server mpcontrol capture can no longer masquerade as MP policy coverage. An mpcontrol source claiming MP production now fails closed as rejected evidence. Refs #328
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/cmtraceopen-parser/tests/sccm_server_management_point.rs (1)
1517-1534: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the requested artifact group for rejected policy evidence.
This test rejects
mp-iis-control-current, which belongs toserver-mp-policy. The assertion checks the classification, the correlation flag, and the cited artifact, but not the requested group.
append_rejected_observationsincrates/cmtraceopen-parser/src/sccm/server/windows/management_point.rsalways requestsserver-mp-auth, so the wrong remediation guidance passes unnoticed. Add the group assertion together with the analyzer fix flagged on lines 1296-1324 of that file.♻️ Proposed additional assertion
let analysis = analysis_value(&bundle); + assert!( + analysis["artifactRequests"] + .as_array() + .expect("artifact requests") + .iter() + .any(|request| request["logicalArtifactId"] == "server-mp-policy"), + "a rejected policy-group record must request the policy group" + ); 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_management_point.rs` around lines 1517 - 1534, Extend the rejected-observation assertion in the test to require the expected requested artifact group, server-mp-policy, alongside the existing classification, correlationEligible, and artifactId checks. Update append_rejected_observations in the management-point analyzer so policy evidence requests server-mp-policy instead of server-mp-auth, preserving the existing rejected-evidence behavior.
🤖 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 1296-1324: Update append_rejected_observations so each branch
derives its requested group from the rejected reference’s owning source and uses
the corresponding group-specific reason text, while retaining exactly one group
per observation for actual_finding_signatures. In
crates/cmtraceopen-parser/tests/sccm_server_management_point.rs:1517-1534, add
an assertion that the rejected mp-iis-control-current record requests the
server-mp-policy artifact group.
---
Nitpick comments:
In `@crates/cmtraceopen-parser/tests/sccm_server_management_point.rs`:
- Around line 1517-1534: Extend the rejected-observation assertion in the test
to require the expected requested artifact group, server-mp-policy, alongside
the existing classification, correlationEligible, and artifactId checks. Update
append_rejected_observations in the management-point analyzer so policy evidence
requests server-mp-policy instead of server-mp-auth, preserving the existing
rejected-evidence behavior.
🪄 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: a791112d-cc5c-48e6-b7a7-439f672c28ae
📒 Files selected for processing (5)
crates/cmtraceopen-parser/src/sccm/server/windows/management_point.rscrates/cmtraceopen-parser/src/sccm/server/windows/mod.rscrates/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/sccm_server_management_point.rs
adamgell
left a comment
adamgell
left a comment
There was a problem hiding this comment.
Independent review, round 4, exact head 4a2d926.
Verdict: PASS.
Descent: merge-base with skeleton 7655826 is 7655826; the diff against the
skeleton is confined to the five lane files (management_point.rs,
server/windows/mod.rs, iis-supplemental expected.json and manifest.json, and
the MP test file).
Battery at the exact head:
- cargo test --test sccm_server_management_point: 25 passed, 0 failed
- cargo test --test sccm_spine_contract: 137 passed, 0 failed
- cargo test --test sccm_server_intake: 11 passed, 0 failed
- full crate cargo test: 938 passed, 0 failed
- cargo clippy --all-targets -- -D warnings: clean
- cargo check --target wasm32-unknown-unknown: clean
Semantic decision: the reconciliation is judged consistent with the merged
intake contract. The shared catalog declares mpcontrol as site-server
produced (family ManagementPoint) and the server catalog declares the
subject-scoped server-mp-policy row; intake tests enforce both the rejection
of an MP-produced mpcontrol claim and the acceptance of the subject-scoped
modeling. Keying is_supplemental_source on catalog identity instead of the
producer string, and excluding supplemental sources from coverage_for_group,
closes a real leak: a site-server mpcontrol capture can no longer turn an
absent server-mp-policy state into parseFailed or accessDenied. Probes
confirm: mpcontrol alongside absent MP logs reports server-mp-policy absent;
an MP-claiming mpcontrol fails closed as cited rejected evidence without
panicking (and the two new tests fail against the pre-fix source, confirming
RED discipline); a genuine MP_GetPolicy.log capture is unaffected.
Recorded design observation, not blocking: mpcontrol evidence currently
contributes nothing to the MP analysis output itself (it is not cited by any
observation and is surfaced only at the intake layer). Issue 328 lists MP
control evidence as input and an MP control/role health failure class to
distinguish; that distinguisher is absent from this slice, as are MP_Status,
MP_Framework, CcmIsapi, CCM_STS, and ClientAuth handling. This matches the
slice scope accepted in prior rounds. Under the merged contract, role health
analysis belongs to a site-server-side workflow keyed on the subject-scoped
catalog row and should be tracked as follow-up work rather than faked here.
Previously resolved threads: the frozen tests for all six resolved threads
pass individually at this head. The one open Minor thread (rejected evidence
always requests the auth group) is confirmed present at this head and
interacts with the new fail-closed path only as an inaccurate remediation
hint; it postdates this head and remains tracked on the PR.
Privacy: the lane changes contain only synthetic identifiers and safe
handles; the fixture flip adds only role and workflowSubject fields; the
pre-existing path canary lives inside the no-export test that asserts it
never appears in output. No new exposure.
Module unions: catalog, intake, and management_point export disjoint name
sets under the three glob re-exports; the removal of pub use server::* from
sccm/mod.rs breaks no import path in the merged tree.
Rejected MP records currently always ask for MP_GetAuth.log even when the rejected record lives in the server-mp-policy group. Freeze the corrected contract: remediation hints must request the owning group of the rejected references, one group per observation, across all three rejection flavors. The unrelated-client-like-key fixture flips to the policy group (its only captured source), which also unmasks the honest absent-auth coverage observation, and rejected observation ids become group-qualified so mixed-group rejections stay unique. Refs #328
append_rejected_observations hardcoded MP_AUTH_GROUP in all three rejection branches, so any rejected policy-family record, including the fail-closed mpcontrol path, told the operator to collect MP_GetAuth.log instead of the artifact that actually holds the evidence. Partition the rejected references by the source group that owns them and emit one observation and finding per group, so each remediation hint requests its own family. Observation and finding ids are group-qualified to stay unique when a bundle rejects records from both groups. Deriving the phase from the owning group keeps it the inverse of group_for_phase, so an observation never cites a phase that maps back to a group it did not request; the same helper replaces the duplicated group-to-phase branches in the explicit-coverage path. Rejection identity, class, and reason move to an enum instead of matched string literals. Because a rejected policy record no longer claims the authentication group, an honest absent-auth coverage observation is no longer masked. Refs #328
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/cmtraceopen-parser/tests/fixtures/sccm/server/management-point/unrelated-client-like-key/expected.json (1)
24-29: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse one classification vocabulary for the unrelated-key record.
Line 24 declares the observation classification
incompatibleKey. Line 29 declares the finding classlowConfidenceSymptomfor the same subject. The projection still passes, becauseexpected_finding_signaturesnormalizeslowConfidenceSymptomtosymptomandbuild_source_local_findingmaps every non-insufficientEvidenceclassification toSymptom. The two labels describe one record, so the fixture reads as if the analyzer changed the classification. Declare the finding class that corresponds toincompatibleKey, or add a comment field that records the intended mapping.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/cmtraceopen-parser/tests/fixtures/sccm/server/management-point/unrelated-client-like-key/expected.json` around lines 24 - 29, The unrelated-key record uses inconsistent classification vocabulary between the observation and finding. Update the finding for subjectId `observation:unrelated-client-like-key:server-mp-policy` to use the class corresponding to `incompatibleKey`, or add the supported comment field documenting that mapping; keep the projection’s existing normalization behavior unchanged.crates/cmtraceopen-parser/src/sccm/server/windows/management_point.rs (1)
1372-1385: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRejected-record reason text drifted from the fixtures.
MpRejection::reasonbuilds strings that include the concrete log basename, but no committed fixture declares that text, and the test projections compare onlylogicalArtifactId. The fixtures therefore document a contract the analyzer no longer emits.
crates/cmtraceopen-parser/src/sccm/server/windows/management_point.rs#L1372-L1385: keep the emitted strings as the single source of truth, and add an assertion on the reason text so the fixtures cannot drift again.crates/cmtraceopen-parser/tests/fixtures/sccm/server/management-point/rotation-boundary/expected.json#L24-L29: replace "Collect a supported-version record containing a complete exact request key." with the emittedMP_GetAuth.logwording on both the observation and the finding.crates/cmtraceopen-parser/tests/fixtures/sccm/server/management-point/unrelated-client-like-key/expected.json#L24-L29: replace "Capture a bounded policy-family source with the exact versioned request key." with the emittedMP_GetPolicy.logwording on both the observation and the finding.🤖 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 1372 - 1385, Update MpRejection::reason in crates/cmtraceopen-parser/src/sccm/server/windows/management_point.rs (lines 1372-1385) to assert the emitted reason text in the relevant tests while keeping these strings as the source of truth. In crates/cmtraceopen-parser/tests/fixtures/sccm/server/management-point/rotation-boundary/expected.json (lines 24-29), update both observation and finding reasons to the emitted MP_GetAuth.log wording. In crates/cmtraceopen-parser/tests/fixtures/sccm/server/management-point/unrelated-client-like-key/expected.json (lines 24-29), update both observation and finding reasons to the emitted MP_GetPolicy.log wording.
🤖 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/management_point.rs`:
- Around line 1372-1385: Update MpRejection::reason in
crates/cmtraceopen-parser/src/sccm/server/windows/management_point.rs (lines
1372-1385) to assert the emitted reason text in the relevant tests while keeping
these strings as the source of truth. In
crates/cmtraceopen-parser/tests/fixtures/sccm/server/management-point/rotation-boundary/expected.json
(lines 24-29), update both observation and finding reasons to the emitted
MP_GetAuth.log wording. In
crates/cmtraceopen-parser/tests/fixtures/sccm/server/management-point/unrelated-client-like-key/expected.json
(lines 24-29), update both observation and finding reasons to the emitted
MP_GetPolicy.log wording.
In
`@crates/cmtraceopen-parser/tests/fixtures/sccm/server/management-point/unrelated-client-like-key/expected.json`:
- Around line 24-29: The unrelated-key record uses inconsistent classification
vocabulary between the observation and finding. Update the finding for subjectId
`observation:unrelated-client-like-key:server-mp-policy` to use the class
corresponding to `incompatibleKey`, or add the supported comment field
documenting that mapping; keep the projection’s existing normalization behavior
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5e09d59d-524d-489d-9fd2-f34171c3a1d1
📒 Files selected for processing (7)
crates/cmtraceopen-parser/src/sccm/server/windows/management_point.rscrates/cmtraceopen-parser/src/sccm/server/windows/mod.rscrates/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/rotation-boundary/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/server/management-point/unrelated-client-like-key/expected.jsoncrates/cmtraceopen-parser/tests/sccm_server_management_point.rs
adamgell
left a comment
adamgell
left a comment
There was a problem hiding this comment.
Independent delta review of 5862eb5 (RED) + 7e6ed7b (GREEN), at exact head
7e6ed7b. Reviewed in a detached worktree at
that exact commit; worktree verified clean before and after all probes.
VERDICT: PASS.
DESCENT
merge-base HEAD origin/codex/parser-family-skeleton = 7655826. The skeleton
(now e56ad38) has advanced 56 commits past that point, touching 79 files.
None of them is management_point.rs, sccm_server_management_point.rs, or any
management-point fixture, and git merge-tree reports zero conflict markers.
The advance does not affect this delta.
BATTERY (cargo test --locked -p cmtraceopen-parser)
--test sccm_server_management_point 26 passed, 0 failed
--test sccm_spine_contract 137 passed, 0 failed
--test sccm_server_intake 11 passed, 0 failed
full crate 939 passed, 0 failed (16 binaries)
cargo clippy --all-targets -- -D warnings clean, exit 0
cargo check --target wasm32-unknown-unknown clean, exit 0
RED DISCIPLINE: VERIFIED
Restoring management_point.rs to its RED state while keeping the GREEN tests
produces 23 passed, 3 failed:
mp_produced_mpcontrol_claims_fail_closed_as_rejected_evidence
left: ["server-mp-auth"] right: ["server-mp-policy"]
rejected_records_request_their_owning_source_group
left: ["server-mp-auth"] right: ["server-mp-policy"]
management_point_reducer_matches_the_frozen_terminal_and_coverage_contracts
unrelated-client-like-key: RED emits one observation
"observation:unrelated-client-like-key" requesting server-mp-auth at phase
resolveLocationOrPolicy; GREEN emits two, the group-qualified
"observation:unrelated-client-like-key:server-mp-policy" plus the unmasked
"observation:coverage:server-mp-auth".
The failures are exactly the reported defect, including the phase/group
mismatch and the masked coverage observation.
PER-CLAIM ADJUDICATION
-
Partition by owning group, one observation and finding per group with
group-qualified ids. CONFIRMED. references_by_group is a
BTreeMap<&'static str, _>, so group iteration is deterministic. A probe
bundle rejecting both groups yields exactly one observation per group,
evidence strictly owned by the group each observation requests (no
cross-group bleed), and no duplicate observation or finding ids. -
MpRejection enum replaces stringly-typed dispatch. CONFIRMED. The six-tuple
is gone; id_stem, classification, and reason now hang off the variant. All
three flavors were exercised per group: rotation-malformed and
unrelated-client-like-key appear in the corpus, and a synthesized
policy-owned malformed record produces
"observation:mp-malformed:server-mp-policy" at phase resolveLocationOrPolicy
requesting server-mp-policy. -
entry_phase_for_group is the inverse of group_for_phase. CONFIRMED as a
right-inverse over the reachable domain: group_for_phase(
entry_phase_for_group(g)) == g for both groups owning_group_for_reference
can return. Asserted for every emitted observation across all nine corpus
scenarios plus the multi-group probe; no observation cites a phase that maps
back to a group other than the one it requests. The latent
unrelated-client-like-key case is fixed. -
append_unconsumed_explicit_coverage deduplicated onto the same helper.
CONFIRMED. Two duplicated group-to-phase if/else blocks are replaced by
entry_phase_for_group. -
anyrather thanfindin the group lookup. CONFIRMED. With an artifact id
duplicated across the auth and policy groups, the serialized analysis is
byte-identical under full source and evidence reordering (4453 bytes both
directions). The committed determinism test also passes across all nine
scenarios.
ADVERSARIAL PROBES
Third source group. The catalog declares three groups, not two:
server-mp-auth, server-mp-policy, and server-mp-iis.
owning_group_for_reference maps only policy versus an auth default, which is
safe because is_supplemental_source returns true for MP_IIS_GROUP before any
rejection push, so an IIS-owned reference can never reach the rejection path.
Verified by re-homing a real captured MP-produced source into the IIS group
with a corrupted version: it is never cited by any observation. This matters
because shared_request returns None for any group other than auth or policy,
so an IIS-group request would have been dropped from the shared spine.
mpcontrol fail-closed path. mp-iis-control-current is declared in
server-mp-policy under the siteServer role. Flipping it to ManagementPoint
makes it non-supplemental, it is rejected, and it requests server-mp-policy.
Id collisions. The stems unrelated-client-like-key, rotation-malformed, and
mp-malformed never collide with the coverage forms observation:coverage:{group}
or finding:mp-coverage:{group}. Uniqueness of every observation and finding id
was asserted empirically across all nine scenarios plus the multi-group probe.
PRIVACY
Group-qualified ids are format! over &'static str constants only. Reason
strings are format! over workflow_log_for_group, a closed two-element set of
literal log basenames, never over artifact display names, paths, or handles.
Under a bundle poisoned with an original_path, a host, and an authorization
header plus query handle in every message, the serialized analysis contains
none of Adam.Gell, LAB-MP01, private.example, private-secret, private_object,
AuthorizationHeader, QueryHandle, "C:\", or a UNC prefix.
4a2d926 RECONCILIATION STILL HOLDS
mp_produced_mpcontrol_claims_fail_closed_as_rejected_evidence,
site_server_mpcontrol_never_shapes_management_point_coverage_states, and
management_point_catalog_declares_every_reducer_source are all green at this
head, as is the full 26-test management point battery.
NON-BLOCKING OBSERVATIONS
a) The rotation flavor is still bundle-global while observations are now
per-group. rotation is computed once outside the loop from all sources, so
a policy source that is fully Captured with fragment_complete = Some(true)
is still labeled observation:rotation-malformed:server-mp-policy when only
the auth group carries the fragment. Because RotationMalformed and Malformed
share a classification, a phase, and a requested group, the operator is
still directed at the correct artifact, so this is wording only. The
unrelated predicate was moved inside the per-group loop; rotation was
not. Filtering by source_group for symmetry would close it.
b) Fixture reason prose is unasserted and now stale. source_local_projection
and expected_finding_signatures compare only observationId, phase,
classification, confidence, correlationEligible, and the request logical
ids. The RED commit set the unrelated-client-like-key reason to "Capture a
bounded policy-family source with the exact versioned request key." while
the code emits "Capture bounded MP_GetPolicy.log evidence with the exact
versioned request key.". Nothing catches the divergence, so expected.json
reason fields should be read as documentation rather than contract.
c) Finding id prefixing is now mixed. finding:mp-rotation-fragments and
finding:mp-coverage:{group} keep the mp- prefix, while rejected findings
became finding:rotation-malformed:{group} and
finding:unrelated-client-like-key:{group}. No grammar contract asserts the
prefix and the spine suite is 137/137, so this is cosmetic.
d) The mp-malformed flavor has no corpus fixture; it had to be synthesized to
exercise it. The RED commit message claims coverage across all three
rejection flavors, but only two appear in the frozen corpus.
None of the above is a demonstrated defect in shipped behavior, so none blocks.
There was a problem hiding this comment.
Pull request overview
Adds a new SCCM server-local “Management Point” reducer (#328) to the parser crate, producing deterministic, privacy-safe analysis outputs (transactions/findings/coverage gaps/artifact requests/counterpart-ready facts) backed by bounded evidence references.
Changes:
- Implemented
analyze_management_pointand associated MP analysis model types undercmtraceopen_parser::sccm::server::windows. - Added a comprehensive frozen-contract integration test suite for MP scenarios (success/failure/deferred/rotation/ownership/coverage).
- Updated fixture manifests/expected outputs for new observation/finding semantics and mpcontrol role ownership modeling.
Reviewed changes
Copilot reviewed 7 out of 7 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 | New server-local Management Point reducer and public analysis output types. |
| crates/cmtraceopen-parser/src/sccm/server/windows/mod.rs | Exposes the new MP module from the windows server namespace. |
| crates/cmtraceopen-parser/tests/sccm_server_management_point.rs | New integration tests validating schema, determinism, citations, privacy, and fail-closed behavior. |
| crates/cmtraceopen-parser/tests/fixtures/sccm/server/management-point/unrelated-client-like-key/expected.json | Updates expected output for coverage and incompatible-key observations/findings. |
| crates/cmtraceopen-parser/tests/fixtures/sccm/server/management-point/rotation-boundary/expected.json | Updates expected IDs for rotation-malformed observation/finding to include owning group. |
| crates/cmtraceopen-parser/tests/fixtures/sccm/server/management-point/iis-supplemental/manifest.json | Models mpcontrol as site-server produced with MP as workflow subject. |
| crates/cmtraceopen-parser/tests/fixtures/sccm/server/management-point/iis-supplemental/expected.json | Updates expected provenance to reflect mpcontrol’s site-server role + workflow subject role. |
Suppressed comments (1)
crates/cmtraceopen-parser/src/sccm/server/windows/management_point.rs:1555
shared_requestmapsserver-mp-policyto a singleSccmArtifactRequest(mpGetPolicy). Since the policy group also includesmpLocation, findings derived from missing/rejected location evidence will still request onlyMP_GetPolicy.log, which can misdirect collection guidance. Consider emitting requests for both logical IDs (mpLocation + mpGetPolicy) or splitting the group so the request can be precise.
fn shared_request(request: &SccmManagementPointArtifactRequest) -> Option<SccmArtifactRequest> {
let logical_id = match request.logical_artifact_id.as_str() {
MP_AUTH_GROUP => "mpGetAuth",
MP_POLICY_GROUP => "mpGetPolicy",
_ => return None,
| fn workflow_log_for_group(group: &str) -> &'static str { | ||
| if group == MP_POLICY_GROUP { | ||
| "MP_GetPolicy.log" | ||
| } else { | ||
| "MP_GetAuth.log" |
The success side of the terminal result check and the overlapping half of evidence quarantine are both open here. A success or deferral marker carrying an explicit failure code still proves its phase, and a wider overlapping line range still outranks the record it overlaps. Refs #321
The success side of the terminal result check and the overlapping half of evidence quarantine are both open here. A success or deferral marker carrying an explicit failure code still proves its phase, and a wider overlapping line range still outranks the record it overlaps. Refs #321
Implements #328 as an independent, server-local Management Point reducer.
Scope:
TDD checkpoints:
f0683bc9: public API contract RED (missing SCCM Server: diagnose management-point client transactions #328 API)270b926b: adversarial evidence-boundary RED (6 passed, 4 failed)252680cc: handoff/identity RED (9 passed, 4 failed)6b34d951: focused GREEN (13 passed)Verification at exact head
6b34d9510162a528b4f7ffd2c00f863c6ae8c1e6:cargo test --locked -p cmtraceopen-parser --test sccm_server_management_point— 13 passedcargo test --locked -p cmtraceopen-parser --test sccm_server_intake— 1 passedcargo test --locked -p cmtraceopen-parser --test sccm_spine_contract— 136 passedcargo test --locked -p cmtraceopen-parser— passed (including 351 unit tests and all SCCM integration targets)cargo clippy --locked -p cmtraceopen-parser --all-targets -- -D warnings— passedcargo +1.88.0 check --locked -p cmtraceopen-parser --target wasm32-unknown-unknown— passedrustfmt --edition 2021 --check— passedjq empty— passedgit diff --check— passedRepository-wide
cargo fmt --check --allstill reports 17 inherited files outside #328; no #328-owned path appears in that output.No live Windows acceptance is claimed. CodeRabbit and an independent exact-head review are required before merge.
Issue: #328
Epic: #317
Summary by CodeRabbit
New Features
Tests