feat(sccm): define optional WSUS supplemental intake contract - #459
Conversation
|
@coderabbitai review\n\nPlease perform a fresh full review of exact head e57da51. This is a bounded pure-parser optional SUP/WSUS catalog/intake/fixture contract; do not infer native collection, semantic diagnosis, or live Windows acceptance. |
📝 WalkthroughWalkthroughThe server source catalog now supports the profile-defined WSUS health source. Intake validates its metadata, topology, handles, version, and synthetic identifiers. Fixtures and tests cover skipped intake, invalid mutations, opaque provenance, and the expanded scenario set. ChangesWSUS source intake
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Intake as SCCM server intake
participant Validator as validate_declared_source_tuple
participant Catalog as declared_server_source_catalog
Intake->>Validator: Validate declared WSUS source tuple
Validator->>Catalog: Resolve server-sup-wsus specification
Catalog-->>Validator: Return profile-defined source contract
Validator-->>Intake: Accept or reject source metadata
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Comment |
github-actions
Bot
added
enhancement
There was a problem hiding this comment.
Pull request overview
This PR extends the SCCM server intake (pure parser) contract to recognize an optional WSUS supplemental source (server-sup-wsus) and validates it via a bounded synthetic fixture, ensuring the source remains coverage-only and fail-closed under tuple mutations.
Changes:
- Adds a new declared server source spec for
server-sup-wsuswithprofileDefinedkind and an exactWsusHealth.jsonbasename contract. - Tightens parser-side validation for the WSUS supplemental “tuple” (roles observed, subject linkage, synthetic fixture immutability).
- Introduces a new synthetic fixture scenario (
supplemental-wsus-skipped) and expands the fixture matrix contract accordingly.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| crates/cmtraceopen-parser/src/sccm/server/windows/catalog.rs | Adds ProfileDefined source kind and declares the server-sup-wsus source spec with an explicit basename constraint. |
| crates/cmtraceopen-parser/src/sccm/server/windows/intake.rs | Enforces WSUS supplemental tuple validation and extends synthetic fixture-safe vocabularies for IDs/kinds/handles/fingerprints. |
| crates/cmtraceopen-parser/tests/sccm_server_intake.rs | Adds targeted tests to ensure the optional WSUS supplemental contract is admitted and mutation attempts fail closed. |
| crates/cmtraceopen-parser/tests/sccm_server_intake_fixture_contract.rs | Updates the expected fixture scenario count to include the new WSUS supplemental scenario. |
| crates/cmtraceopen-parser/tests/fixtures/sccm/server/intake/supplemental-wsus-skipped/* | Adds new synthetic WSUS supplemental skipped fixture manifest and expected assessment output. |
|
@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 8 minutes. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
…intake-contract-r122
|
@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 4 minutes. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/cmtraceopen-parser/src/sccm/server/windows/intake.rs (1)
2307-2347: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTrim dead conditions and avoid the hardcoded source-id gate in
validate_declared_source_tuple.Four conditions in this function are unreachable given the calling context in
normalize_artifact:
spec.source_kind != SccmServerSourceKind::ProfileDefined: the catalog has exactly onesource_id == "server-sup-wsus"entry, and itssource_kindis fixed toProfileDefined.artifact.producer_role != SccmRole::WsUs:classify_declared_server_source's lookup already requiresspec.producer_role == producer_rolefor thisspecto be selected.subject.role != SccmRole::SoftwareUpdatePoint: the same lookup already requiresspec.workflow_subject_role == workflow_subject_role, which forces bothworkflow_subjectto beSomeand its role to match.artifact.producer_host_handle.is_none(): this case already returnsInvalidArtifactearlier innormalize_artifact(theproducer_host_handle.is_none()branch around line 1043), before classification even runs.The genuinely load-bearing checks are the
roles_observedmembership check,subject.instance_handle.is_none(), andsource_version.is_none(), plus the synthetic literal-equality block. Keeping the dead conditions increases cognitive load for readers trying to determine which invariants are actually enforced here versus already guaranteed upstream.Separately, gating this function on
spec.source_id != "server-sup-wsus"couples it to a string literal that must be kept in sync by hand with the catalog entry incatalog.rs. If a future rename or typo desynchronizes them, this function silently stops enforcing the WSUS-specific requiredness rules (mandatory subject handle, mandatory source version, topology role membership) while still passing the generic catalog match. Consider gating onspec.source_kind == SccmServerSourceKind::ProfileDefinedinstead, and consider moving the synthetic-fixture literal constants (host handle, subject handle, version, path fingerprint, lineage id) ontoSccmServerSourceSpecso future profile-defined sources don't require a new hardcoded branch in this function.♻️ Proposed fix to remove the confirmed-dead conditions
let subject = artifact .workflow_subject .as_ref() .ok_or(SccmServerIntakeError::InvalidArtifact)?; - if spec.source_kind != SccmServerSourceKind::ProfileDefined - || artifact.producer_role != SccmRole::WsUs - || subject.role != SccmRole::SoftwareUpdatePoint - || !roles_observed.contains(&SccmRole::WsUs) + if !roles_observed.contains(&SccmRole::WsUs) || !roles_observed.contains(&SccmRole::SoftwareUpdatePoint) - || artifact.producer_host_handle.is_none() || subject.instance_handle.is_none() || source_version.is_none() { return Err(SccmServerIntakeError::InvalidArtifact); }If any of these are intentional defense-in-depth against future changes to
classify_declared_server_source, keep them but add a comment stating that intent, so readers don't mistake them for load-bearing checks.🤖 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 2307 - 2347, Update validate_declared_source_tuple to gate profile-specific validation on spec.source_kind rather than the hardcoded "server-sup-wsus" source ID, and remove the upstream-guaranteed checks for source kind, producer role, subject role, and producer host presence. Retain the roles_observed, subject.instance_handle, source_version, and synthetic-fixture validations; if any removed checks are intentionally defensive, keep them only with a comment documenting that purpose.
🤖 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/tests/fixtures/sccm/server/intake/supplemental-wsus-skipped/expected.json`:
- Line 3: Update the supplemental WSUS skipped fixture assertion for the
coverage entry so it validates a field present in the real SccmServerCoverage
output rather than the unsupported requiredness value; if no suitable output
field exists, remove requiredness from the expected fixture.
---
Nitpick comments:
In `@crates/cmtraceopen-parser/src/sccm/server/windows/intake.rs`:
- Around line 2307-2347: Update validate_declared_source_tuple to gate
profile-specific validation on spec.source_kind rather than the hardcoded
"server-sup-wsus" source ID, and remove the upstream-guaranteed checks for
source kind, producer role, subject role, and producer host presence. Retain the
roles_observed, subject.instance_handle, source_version, and synthetic-fixture
validations; if any removed checks are intentionally defensive, keep them only
with a comment documenting that purpose.
🪄 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: edf8dbda-26bb-4bf9-a7aa-ebd52ebaffab
📒 Files selected for processing (6)
crates/cmtraceopen-parser/src/sccm/server/windows/catalog.rscrates/cmtraceopen-parser/src/sccm/server/windows/intake.rscrates/cmtraceopen-parser/tests/fixtures/sccm/server/intake/supplemental-wsus-skipped/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/server/intake/supplemental-wsus-skipped/manifest.jsoncrates/cmtraceopen-parser/tests/sccm_server_intake.rscrates/cmtraceopen-parser/tests/sccm_server_intake_fixture_contract.rs
|
Review disposition for exact correction head 5156400:
The branch was fast-forwarded without force. Fresh CodeRabbit, Copilot, and hosted CI must complete on 5156400 before this draft can be considered for merge. |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/cmtraceopen-parser/src/sccm/server/windows/intake.rs (1)
2307-2346: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe role and kind checks repeat the catalog match.
classify_declared_server_sourcealready matchedspec.producer_role,spec.workflow_subject_role, andspec.source_kindagainst the artifact before this function runs. Lines 2322-2324 therefore cannot fail for a spec that reached this point. The checks that add value are theroles_observedtopology checks, the handle presence checks, and thesource_versionpresence check. Consider keeping only those, and adding a short comment that marks the remaining checks as defense in depth if you want to retain them.This is optional. The current code is correct and fails closed.
🤖 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 2307 - 2346, Simplify validate_declared_source_tuple by removing the redundant spec.source_kind, artifact.producer_role, and subject.role comparisons already guaranteed by classify_declared_server_source; retain the roles_observed, handle-presence, and source_version checks. If those defense-in-depth checks remain necessary, document that intent with a brief comment.
🤖 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/intake.rs`:
- Around line 2307-2346: Simplify validate_declared_source_tuple by removing the
redundant spec.source_kind, artifact.producer_role, and subject.role comparisons
already guaranteed by classify_declared_server_source; retain the
roles_observed, handle-presence, and source_version checks. If those
defense-in-depth checks remain necessary, document that intent with a brief
comment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 780eb03e-95e4-4ff6-8b99-50826df40f34
📒 Files selected for processing (6)
crates/cmtraceopen-parser/src/sccm/server/windows/catalog.rscrates/cmtraceopen-parser/src/sccm/server/windows/intake.rscrates/cmtraceopen-parser/tests/fixtures/sccm/server/intake/supplemental-wsus-skipped/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/server/intake/supplemental-wsus-skipped/manifest.jsoncrates/cmtraceopen-parser/tests/sccm_server_intake.rscrates/cmtraceopen-parser/tests/sccm_server_intake_fixture_contract.rs
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (1)
crates/cmtraceopen-parser/src/sccm/server/windows/intake.rs:2340
- For the
server-sup-wsussynthetic fixture gate,validate_declared_source_tuplefreezes lineage/path/handles/version but does not constrainartifact.rotation.kind/artifact.rotation.value. As a result, a synthetic WSUS manifest can mutate the rotation shape (e.g.,kind: "timestamped"with avalue) and still be accepted, undermining the intended fail-closed contract for this exact tuple.
if synthetic_fixture
&& (artifact.producer_host_handle.as_deref() != Some("synthetic:host:wsus-01")
|| subject.instance_handle.as_deref() != Some("synthetic:subject:sup-01")
|| source_version != Some("5.00.TEST")
|| artifact.configured_path_provenance.path_fingerprint
!= "synthetic:path:sup-wsus-health"
|| artifact.rotation.lineage_id != "sup-wsus-health")
|
Exact-head Copilot cycle completed on 5156400 with no new thread, but its suppressed rotation-shape observation is being treated as a real merge gate. Independent code tracing shows the profileDefined branch calls validate_declared_source_tuple, which freezes only the WSUS lineage identifier, then canonicalizes the source with no parsed rotation. A valid timestamped kind/value can therefore be accepted while the declared providerDefined shape is discarded. The draft remains NO-GO. A separate TDD correction will require the exact synthetic WSUS providerDefined kind with no value and preserve production opaque provenance behavior; fresh independent review and hosted cycles will follow. |
|
@coderabbitai review Exact head |
|
✅ Action performedFull review finished. |
* feat(sccm): catalogue optional WSUS supplemental intake * test(sccm): expose WSUS supplemental tuple gaps * fix(sccm): validate WSUS supplemental tuple * test(sccm): align WSUS coverage oracle with serialized output * fix(sccm): freeze synthetic WSUS rotation tuple
Scope
Bounded pure-parser SCCM Server intake/catalog/fixture contract for the optional
server-sup-wsussupplemental source. This adds no native collector, Windows I/O, diagnosis reducer, broad parser, or live acceptance claim.Contract
WsusHealth.jsonsource withwsUsproducer andsoftwareUpdatePointworkflow subject.5.00.TESTfixture/version, configured-path fingerprint, role topology, host/subject handles, lineage, exact provider-defined rotation shape, and version/profile provenance.Exact head and base
Head:
a1de6d76d6ca23682a5d0d3583acd2e5d5663433Base:
bc5d4f854362e31ffbfae46d9a07950955690887The branch was fast-forwarded without force from
51564000dbca5087df4396a5272bb86f872d2184. The new commit changes exactly server intake validation and its focused tests.Review corrections
requiredness, which is not serialized onSccmServerCoverage; a regression proves every expected WSUS coverage key exists in the serialized assessment.providerDefinedwith no value. Focused RED tests proved that a valid timestamped shape and a provider-defined value were previously accepted and then discarded asrotation: None; both now fail closed.server-sup-wsus && synthetic_fixture. Generic profile-defined parsing and non-synthetic production-style WSUS provenance are unchanged.server-sup-wsusguard remains intentional. Broadening it to allprofileDefinedsources would impose WSUS-only role, subject, version, host, path, and rotation constraints on future source cards.Verification on exact head
cargo test -p cmtraceopen-parser --test sccm_server_intake— 68/68npx tsc --noEmit— passrustfmt --check— passgit diff --check— passFresh hosted CodeRabbit, Copilot, and CI are required on exact head
a1de6d76before merge. Earlier hosted results are not counted for this head.Dependencies and non-goals
Native configured-path discovery/capture, native Windows/server-lab validation, semantic SUP/WSUS diagnosis, and client/server correlation remain separate follow-on work. A missing default path is not evidence a role is absent or broken.
Closes none; the server-intake issue remains open.
Summary by CodeRabbit
New Features
Bug Fixes
Tests