fix(sccm): bind server coverage to canonical topology - #455
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughSCCM server intake now preserves optional producer-host and workflow-subject handles. Integrity and aggregation keys include these handles. Site-core analysis validates intake-bound topology and reports explicit gaps for incongruent coverage. ChangesSCCM coverage topology
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Manifest
participant ServerIntake
participant SiteCore
Manifest->>ServerIntake: provide assessment and payload manifests
ServerIntake->>ServerIntake: validate topology and normalize coverage handles
ServerIntake->>SiteCore: provide intake-bound coverage
SiteCore->>SiteCore: compare topology-aware coverage
SiteCore-->>ServerIntake: return analysis or explicit authority gaps
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Comment |
|
@coderabbitai full review Please perform a fresh hosted full review of exact head |
|
✅ Action performedFull review finished. |
github-actions
Bot
added
bug
There was a problem hiding this comment.
Pull request overview
This PR hardens SCCM server intake “coverage” so each coverage row is bound to the canonical, intake-validated topology (producer host + optional workflow subject), preventing distinct physical topology from collapsing into a single coverage row and ensuring downstream reducers fail closed when topology is mutated post-intake.
Changes:
- Bind normalized schema-v1 coverage grouping to
(producer role, producerHostHandle, sourceId, workflowSubjectRole, workflowSubjectHandle, capture state)and omit the new handle fields from JSON when absent. - Make site-core analysis require an intake-bound topology authority and congruent coverage before emitting normal results; otherwise emit explicit bounded coverage gaps/findings.
- Add targeted adversarial RED/GREEN tests plus documentation and changelog notes for the additive schema-v1 fields.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| docs/sccm/preparation/issue-335-server-intake.md | Documents the new topology-bound coverage grouping rules and additive schema-v1 handle fields. |
| crates/cmtraceopen-parser/src/sccm/server/windows/intake.rs | Adds optional topology handle fields to SccmServerCoverage, includes them in coverage grouping, and binds topology authority to intake integrity. |
| crates/cmtraceopen-parser/src/sccm/server/windows/site_core.rs | Requires intake-bound topology authority + congruent coverage before results can be shaped by site-core reduction. |
| crates/cmtraceopen-parser/src/sccm/server/windows/management_point_tests.rs | Adds fixture helpers and new tests ensuring the MP adapter respects producer-host/workflow-subject-distinguished coverage and rejects post-intake mutations. |
| crates/cmtraceopen-parser/tests/sccm_server_intake.rs | Adds serialization and determinism tests proving coverage rows are distinguished by producer host and workflow subject, and that absent optional fields are omitted in JSON. |
| crates/cmtraceopen-parser/tests/sccm_server_site_core.rs | Adds site-core tests asserting incongruent topology/coverage fails closed into bounded gaps/observations/findings/requests. |
| CHANGELOG.md | Notes the SCCM server coverage topology fix and the fail-closed behavior for incongruent coverage. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/cmtraceopen-parser/src/sccm/server/windows/site_core.rs (1)
304-329: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winBind Site Core admission to the sealed intake integrity
site_core_coverage_is_congruentcompares mutable artifacts and coverage only. A synchronizedproducer_host_handlechange keeps this check true, whiletopology_authority_is_intake_bound()remains true. Calladapter_authority_is_intake_bound()or an equivalent per-record check before admitting sources. Add a regression test for this mutation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/cmtraceopen-parser/src/sccm/server/windows/site_core.rs` around lines 304 - 329, Update SiteCoreContext::new so source admission also requires sealed adapter-authority integrity, not only topology_authority_is_intake_bound() and site_core_coverage_is_congruent(). Invoke adapter_authority_is_intake_bound() or an equivalent per-record validation before admitted_sources, and add a regression test that mutates producer_host_handle while verifying the source is rejected.
🧹 Nitpick comments (2)
crates/cmtraceopen-parser/src/sccm/server/windows/intake.rs (2)
197-224: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared topology normalization to avoid drift.
topology_authority_is_intake_boundre-implements the clone-sort-dedup block forroles_observedthat already exists incanonical_intake_integrity_with_structure(around lines 1490-1500). Both blocks must stay behaviorally identical for the fail-closed guarantee to hold. If one block's duplicate-detection or sort key changes without the matching update in the other, the two authority checks can silently diverge.Extract a shared helper, for example:
fn normalized_topology_or_none( topology: &SccmServerTopologyAssessment, ) -> Option<SccmServerTopologyAssessment> { let mut normalized = topology.clone(); normalized .roles_observed .sort_by(|left, right| role_sort_key(left).cmp(role_sort_key(right))); if normalized.roles_observed.windows(2).any(|roles| roles[0] == roles[1]) { return None; } Some(normalized) }Call it from both
topology_authority_is_intake_bound(mapNonetofalse) andcanonical_intake_integrity_with_structure(propagate with?).As per coding guidelines, "Keep components modular and concerns clearly separated."
🤖 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 197 - 224, Extract the shared roles_observed clone, sort, and duplicate-validation logic into a helper such as normalized_topology_or_none, returning None when duplicate roles are found. Replace the inline normalization in topology_authority_is_intake_bound with this helper and map None to false; update canonical_intake_integrity_with_structure to call the same helper and propagate None with ?. Preserve the existing role_sort_key ordering and fail-closed behavior.Source: Coding guidelines
641-644: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUnify the three coverage-identity representations.
coverage_by_key,CoverageIntegrityIdentity, andsite_core.rs'sCoverageKeyall encode the same(producer_role, producer_host_handle, workflow_subject_role, workflow_subject_handle, source_id, state)identity independently. Extract one shared type (for example aCoverageIdentityKeystruct derivingOrd) and reuse it at every site, so a future field addition to the coverage schema cannot update one representation while missing the others.
crates/cmtraceopen-parser/src/sccm/server/windows/intake.rs#L641-L644: buildcoverage_by_key's key from the shared type instead of a positional 6-tuple.crates/cmtraceopen-parser/src/sccm/server/windows/intake.rs#L285-L289: replaceCoverageIntegrityIdentitywith the shared type (or make it the shared type that the other two sites import).crates/cmtraceopen-parser/src/sccm/server/windows/intake.rs#L1541-L1553: build the identity via the shared type's constructor instead of a separate struct literal.crates/cmtraceopen-parser/src/sccm/server/windows/site_core.rs#L1938-L1945: replace the localCoverageKeytype alias with the shared type, deciding once whetherNonehandles collapse to an empty string or stayOption<String>, and apply that choice consistently everywhere.🤖 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 641 - 644, Unify the duplicated coverage identity representations by introducing one shared Ord-capable CoverageIdentityKey and using it consistently: update intake.rs lines 641-644 to key coverage_by_key with it, intake.rs lines 285-289 to replace or define CoverageIntegrityIdentity as that shared type, intake.rs lines 1541-1553 to construct identities through its constructor, and site_core.rs lines 1938-1945 to replace CoverageKey. Choose one consistent representation for None handles—empty strings or Option<String>—and apply it at every site.
🤖 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.
Outside diff comments:
In `@crates/cmtraceopen-parser/src/sccm/server/windows/site_core.rs`:
- Around line 304-329: Update SiteCoreContext::new so source admission also
requires sealed adapter-authority integrity, not only
topology_authority_is_intake_bound() and site_core_coverage_is_congruent().
Invoke adapter_authority_is_intake_bound() or an equivalent per-record
validation before admitted_sources, and add a regression test that mutates
producer_host_handle while verifying the source is rejected.
---
Nitpick comments:
In `@crates/cmtraceopen-parser/src/sccm/server/windows/intake.rs`:
- Around line 197-224: Extract the shared roles_observed clone, sort, and
duplicate-validation logic into a helper such as normalized_topology_or_none,
returning None when duplicate roles are found. Replace the inline normalization
in topology_authority_is_intake_bound with this helper and map None to false;
update canonical_intake_integrity_with_structure to call the same helper and
propagate None with ?. Preserve the existing role_sort_key ordering and
fail-closed behavior.
- Around line 641-644: Unify the duplicated coverage identity representations by
introducing one shared Ord-capable CoverageIdentityKey and using it
consistently: update intake.rs lines 641-644 to key coverage_by_key with it,
intake.rs lines 285-289 to replace or define CoverageIntegrityIdentity as that
shared type, intake.rs lines 1541-1553 to construct identities through its
constructor, and site_core.rs lines 1938-1945 to replace CoverageKey. Choose one
consistent representation for None handles—empty strings or Option<String>—and
apply it at every site.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 972c26b7-8119-402e-adb9-23068ab09398
📒 Files selected for processing (7)
CHANGELOG.mdcrates/cmtraceopen-parser/src/sccm/server/windows/intake.rscrates/cmtraceopen-parser/src/sccm/server/windows/management_point_tests.rscrates/cmtraceopen-parser/src/sccm/server/windows/site_core.rscrates/cmtraceopen-parser/tests/sccm_server_intake.rscrates/cmtraceopen-parser/tests/sccm_server_site_core.rsdocs/sccm/preparation/issue-335-server-intake.md
|
Hosted CodeRabbit review |
|
@coderabbitai full review Please perform a fresh hosted full review of exact head 670954e against base c622c9e after the independently reviewed final authority-clarity commit. |
|
Adjudicated all four exact-b214 hosted CodeRabbit nits in final reviewed commit 670954e. Applied: shared MP fixture JSON loading and a structurally hoisted authority guard, eliminating the empty-index dependency. Clarified: canonical source values are deliberately forbidden after any seal failure. Retained by design: the explicit topology-authority gate, because it fails closed at the exact point topology scopes facts and protects against future full-seal contract drift. Broad helper collapse was rejected because the existing helpers preserve distinct attack surfaces. Independent exact-delta review: GO/no P0-P3. Site Core 42, MP units 42, MP integration 3, full parser, strict Clippy, wasm32, TypeScript, scoped Rustfmt/diff passed; local CodeRabbit returned zero findings. |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/cmtraceopen-parser/tests/sccm_server_site_core.rs (2)
1783-1793: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
assert_intake_authority_mutation_fails_closedfor the canonical triples.This call passes the same triple list that
assert_intake_authority_mutation_fails_closedalready encodes at lines 403-413. The same literal also repeats at lines 1829-1836. If a fixture artifact id or host handle changes, three sites must change together.Call the existing helper at both sites and keep the explicit triple lists only where the forged or swapped values differ, such as lines 1661-1671 and 1756-1766.
♻️ Proposed fix
let analysis = analyze_site_core(&assessment); - assert_topology_incongruence_fails_closed( - &analysis, - &[ - ( - "sitecomp-current", - "server-sitecomp", - "synthetic:host:site-01", - ), - ("z-site-status", "server-status", "synthetic:host:site-01"), - ], - ); + assert_intake_authority_mutation_fails_closed(&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/tests/sccm_server_site_core.rs` around lines 1783 - 1793, Replace the duplicated canonical triple list in the assert_topology_incongruence_fails_closed call with assert_intake_authority_mutation_fails_closed, and make the same substitution for the repeated list around the second referenced site. Retain explicit triple lists only for forged or swapped values that differ from the helper’s canonical triples.
1826-1838: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInclude the mutation label in the assertion output.
The loop discards the label at line 1826. If one of the three topology mutations stops failing closed, the panic message does not identify which one. Pass the label into the assertion helper or add it to a per-case message.
♻️ Proposed fix
- for (_, analysis) in &analyses { + for (mutation, analysis) in &analyses { + assert!( + analysis.results.is_empty(), + "{mutation} mutation still produced site-core results" + ); assert_topology_incongruence_fails_closed( 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/tests/sccm_server_site_core.rs` around lines 1826 - 1838, Preserve the mutation label while iterating over analyses instead of discarding it in the loop, and pass it to assert_topology_incongruence_fails_closed or include it in a per-case assertion message so failures identify the specific topology mutation.
🤖 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_server_site_core.rs`:
- Around line 1783-1793: Replace the duplicated canonical triple list in the
assert_topology_incongruence_fails_closed call with
assert_intake_authority_mutation_fails_closed, and make the same substitution
for the repeated list around the second referenced site. Retain explicit triple
lists only for forged or swapped values that differ from the helper’s canonical
triples.
- Around line 1826-1838: Preserve the mutation label while iterating over
analyses instead of discarding it in the loop, and pass it to
assert_topology_incongruence_fails_closed or include it in a per-case assertion
message so failures identify the specific topology mutation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f5e8c608-653f-4cdd-a7d5-873ce8b871ab
📒 Files selected for processing (7)
CHANGELOG.mdcrates/cmtraceopen-parser/src/sccm/server/windows/intake.rscrates/cmtraceopen-parser/src/sccm/server/windows/management_point_tests.rscrates/cmtraceopen-parser/src/sccm/server/windows/site_core.rscrates/cmtraceopen-parser/tests/sccm_server_intake.rscrates/cmtraceopen-parser/tests/sccm_server_site_core.rsdocs/sccm/preparation/issue-335-server-intake.md
|
@coderabbitai full review Please perform a fresh hosted full review of exact head efbbb46. The final two exact-670 test-maintenance findings are addressed in one test-only commit. Live integration has advanced to discovery squash 1bba619; the local merge-tree is conflict-free and preserves the two discovery files plus all seven server files byte-for-byte. |
|
✅ 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 7 minutes. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/cmtraceopen-parser/src/sccm/server/windows/management_point_tests.rs (1)
866-867: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the test to match its assertion.
The name says
accepts, but the test requiresErr(SccmManagementPointIntakeError::TopologyMismatch)for both the original and the reordered assessment. The verified property is that distinct producer-host rows stay intake-bound and reach Management Point topology validation.♻️ Proposed rename
-fn canonical_intake_adapter_accepts_coverage_rows_distinguished_by_producer_host() { +fn canonical_intake_coverage_rows_distinguished_by_producer_host_reach_topology_validation() {🤖 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 866 - 867, Rename the test function canonical_intake_adapter_accepts_coverage_rows_distinguished_by_producer_host to describe that distinct producer-host rows remain intake-bound and trigger Management Point topology validation, specifically asserting TopologyMismatch for both original and reordered assessments.crates/cmtraceopen-parser/tests/sccm_server_site_core.rs (1)
400-414: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBind the forbidden values to the assessment under test.
assert_intake_authority_mutation_fails_closedhardcodes the artifact IDs, source IDs, and producer host of the sitecomp/status fixture pair. Every current caller uses that pair, so the assertion is valid today. If a future caller uses a different fixture, the helper will assert absence of strings that the analysis never held, and the test will pass without checking its own values.Pass the assessment, or the expected triples, so the assertion always matches the data under test.
♻️ Proposed direction
-fn assert_intake_authority_mutation_fails_closed(analysis: &SccmSiteCoreAnalysis) { +fn assert_intake_authority_mutation_fails_closed( + analysis: &SccmSiteCoreAnalysis, + intake: &SccmServerIntakeAssessment, +) { // Once the intake seal fails, even the original canonical source values // are no longer authority and must not survive the constant quarantine. - assert_topology_incongruence_fails_closed( - analysis, - &[ - ( - "sitecomp-current", - "server-sitecomp", - "synthetic:host:site-01", - ), - ("z-site-status", "server-status", "synthetic:host:site-01"), - ], - ); + let triples = intake + .artifacts + .iter() + .map(|artifact| { + ( + artifact.artifact_id.as_str(), + artifact.source_id.as_str(), + artifact.producer_host_handle.as_deref().unwrap_or_default(), + ) + }) + .collect::<Vec<_>>(); + assert_topology_incongruence_fails_closed(analysis, &triples); }Also consider renaming
assert_topology_incongruence_fails_closed. Callers use it for evidence, rotation, and identity mutations, not only topology incongruence.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/cmtraceopen-parser/tests/sccm_server_site_core.rs` around lines 400 - 414, Update assert_intake_authority_mutation_fails_closed and its callers so the forbidden artifact IDs, source IDs, and producer hosts are derived from or explicitly passed with the SccmSiteCoreAnalysis under test, then forward those values to assert_topology_incongruence_fails_closed instead of hardcoding the fixture pair. Rename assert_topology_incongruence_fails_closed to a mutation-appropriate name reflecting its use for evidence, rotation, and identity cases, and update all references.
🤖 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_tests.rs`:
- Around line 866-867: Rename the test function
canonical_intake_adapter_accepts_coverage_rows_distinguished_by_producer_host to
describe that distinct producer-host rows remain intake-bound and trigger
Management Point topology validation, specifically asserting TopologyMismatch
for both original and reordered assessments.
In `@crates/cmtraceopen-parser/tests/sccm_server_site_core.rs`:
- Around line 400-414: Update assert_intake_authority_mutation_fails_closed and
its callers so the forbidden artifact IDs, source IDs, and producer hosts are
derived from or explicitly passed with the SccmSiteCoreAnalysis under test, then
forward those values to assert_topology_incongruence_fails_closed instead of
hardcoding the fixture pair. Rename assert_topology_incongruence_fails_closed to
a mutation-appropriate name reflecting its use for evidence, rotation, and
identity cases, and update all references.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0fa967e0-41ea-45a2-b7d3-c601f2d3455d
📒 Files selected for processing (7)
CHANGELOG.mdcrates/cmtraceopen-parser/src/sccm/server/windows/intake.rscrates/cmtraceopen-parser/src/sccm/server/windows/management_point_tests.rscrates/cmtraceopen-parser/src/sccm/server/windows/site_core.rscrates/cmtraceopen-parser/tests/sccm_server_intake.rscrates/cmtraceopen-parser/tests/sccm_server_site_core.rsdocs/sccm/preparation/issue-335-server-intake.md
|
@coderabbitai full review |
|
Exact head advanced to
Exact current-source gates: Site Core 42/42, renamed MP unit 1/1, full parser, strict all-target parser Clippy, parser wasm32, TypeScript, scoped Rustfmt/diff, local CodeRabbit zero findings, and independent follow-up GO/no P0-P3. Production code is unchanged by both follow-ups. Live integration is still All prior hosted checks and reviews are stale for merge acceptance. Fresh exact-head CI, full CodeRabbit, Copilot, zero unresolved threads, and final live merge-ref verification are required. |
✅ 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 4 minutes. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (1)
crates/cmtraceopen-parser/src/sccm/server/windows/intake.rs:580
SccmServerCoverageis a public struct re-exported fromcmtraceopen_parser::sccm::server::windows, and adding new fields (producer_host_handle,workflow_subject_handle) is a semver-breaking Rust API change for downstream crates that construct or destructure this type. If SCCM server intake is intended to be a stable public API, consider either (a) marking these schema structs#[non_exhaustive]going forward (and providing constructors/builders if external construction is needed), or (b) planning a major version bump and calling out the Rust-level breaking change explicitly in release notes.
#[derive(Debug, Clone, PartialEq, Serialize)]
#[serde(rename_all = "camelCase")]
pub struct SccmServerCoverage {
pub producer_role: SccmRole,
#[serde(skip_serializing_if = "Option::is_none")]
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/cmtraceopen-parser/src/sccm/server/windows/site_core.rs (1)
338-342: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the redundant topology seal.
adapter_authority_is_intake_bound()already validates the same normalized topology digest before this code runs. No independent callers or tests use the topology seal. Callsite_core_coverage_is_congruent(intake)directly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/cmtraceopen-parser/src/sccm/server/windows/site_core.rs` around lines 338 - 342, Remove the redundant topology-authority check from the coverage calculation and assign coverage_congruent directly from site_core_coverage_is_congruent(intake). Update the surrounding comment to no longer describe an independent topology seal.Source: Coding guidelines
🤖 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/site_core.rs`:
- Around line 338-342: Remove the redundant topology-authority check from the
coverage calculation and assign coverage_congruent directly from
site_core_coverage_is_congruent(intake). Update the surrounding comment to no
longer describe an independent topology seal.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: aa0c28d8-c4e2-4af3-bdbd-c45c1145fa59
📒 Files selected for processing (7)
CHANGELOG.mdcrates/cmtraceopen-parser/src/sccm/server/windows/intake.rscrates/cmtraceopen-parser/src/sccm/server/windows/management_point_tests.rscrates/cmtraceopen-parser/src/sccm/server/windows/site_core.rscrates/cmtraceopen-parser/tests/sccm_server_intake.rscrates/cmtraceopen-parser/tests/sccm_server_site_core.rsdocs/sccm/preparation/issue-335-server-intake.md
|
Exact-head CodeRabbit disposition for review 4847034460:
This is a non-actionable maintainability preference, not an unresolved correctness finding. Exact head remains 5bd5768; no code or CI state was invalidated. Windows packaging is still the only hosted merge gate. |
* test(parser): cover server coverage topology handles * fix(parser): bind server coverage to topology handles * test(parser): reject incongruent server coverage topology * fix(parser): enforce server coverage topology congruence * test(sccm): expose MP coverage handle seal gaps * fix(sccm): seal MP coverage topology handles * test(sccm): reject mutated site-core topology authority * fix(sccm): bind site-core to intake topology authority * test(sccm): reject forged site-core producer authority * fix(sccm): seal site-core intake authority * refactor(sccm): share topology authority normalization * refactor(sccm): centralize coverage identity keys * test(sccm): quarantine invalid intake authority * fix(sccm): quarantine invalid intake authority * refactor(sccm): clarify server authority quarantine * test(sccm): clarify server authority mutations * test(sccm): bind authority assertions to intake * test(sccm): keep authority exclusions nonvacuous
Summary
handles as well as role/source/state, preventing distinct physical topology
from collapsing into one row.
authority. Post-intake mutations to site, capture-host, observed-role, or
coverage handles now fail closed into bounded coverage evidence rather than
normal diagnostic facts.
public assessment field. An invalid seal produces one fixed, unscoped
intake-authority-invalidcoverage marker with no requests, findings,evidence, results, correlation, or caller-derived identifiers.
gaps, site-core topology mutations, and forged-scope quarantine; document the
additive schema-v1 coverage fields.
Evidence and boundaries
patch-equivalent replay plus hosted-review RED/GREEN authority correction and
private normalization/coverage-key refactors.
no native collection, Windows I/O, database/network/Tauri dependency,
ParserKind::Sccm,LogEntrychange, or genericArtifactStatuschange.omitted. SCCM server intake is unreleased on
main; its first public releasemust preserve these fields as its contract.
acceptance, live collection, client/server correlation, or causal diagnosis.
Verification
cargo test --locked -p cmtraceopen-parser --test sccm_server_intake— 62passed
cargo test --locked -p cmtraceopen-parser --test sccm_server_site_core— 42passed
cargo test --locked -p cmtraceopen-parser management_point_tests— 42passed
cargo test --locked -p cmtraceopen-parser --test sccm_server_management_point— 3 passed
cargo test --locked -p cmtraceopen-parser— passedcargo clippy --locked -p cmtraceopen-parser --all-targets -- -D warnings—passed
cargo check --locked -p cmtraceopen-parser --target wasm32-unknown-unknown— passed
tsc --noEmit, andgit diff --check—passed
Repository-wide formatting still has inherited unrelated drift; this range adds
none.
Review gate
Hosted full CodeRabbit on the first published head found that Site Core checked
topology congruence without the complete adapter seal. The first correction
stopped facts but was independently rejected because forged post-intake handles
could still scope fallback coverage and requests. RED
749ebc94and GREENb21419a4quarantine invalid authority before any mutable artifact/evidenceread. Independent exact-head re-review returned GO/no P0-P3; authenticated local
CodeRabbit full-range and correction-delta reviews both completed with zero
findings. Hosted CodeRabbit on
b21419a4then raised four trivial maintainability items.670954e2reuses the MP JSON helper, hoists the invariant authority guard outside evidence iteration, documents the deliberate topology defense-in-depth gate, and clarifies forbidden-source test naming. Broad helper collapse and removal of the explicit topology gate were independently rejected because they obscure distinct attack surfaces and weaken a non-negotiable topology boundary. Independent exact-delta review and local CodeRabbit returned GO/zero findings. Hosted exact670954e2then identified two valid test-maintenance nits. Test-onlyefbbb46ereuses the canonical authority helper and retains topology-mutation labels in assertion failures; independent exact-delta review, focused Site Core 42/42, changed-file Rustfmt, diff checks, and local CodeRabbit all passed with no finding. Integration then advanced through reviewed discovery squash1bba619f; its two changed discovery files are disjoint from this seven-file server range, and merge-tree7d179a87582d3c211a24b1bef09055c1b5760c02preserves both sides byte-for-byte without conflict. This draft remains unmerged until fresh hosted full CodeRabbit,hosted Copilot, exact-head CI, and the normal final merge guard confirm
base/head/thread state. If #391 or #407 lands first, this branch must be
restacked and re-reviewed because both touch server intake paths.
Tracking
Part of #317; advances #335 and protects downstream #327/#328 reducers. It does
not complete any of those issues.
Summary by CodeRabbit
Bug Fixes
Documentation
producerHostHandleandworkflowSubjectHandlefields.