fix(sccm): preserve bounded discovery coverage correctness - #454
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 discovery separates declarations from bounded coverage diagnostics. It validates raw identities, preserves rejected observations, supports skipped states, applies declaration limits after selection, and reports deterministic omission and provenance issues. ChangesSCCM discovery coverage
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant SCCMObservations
participant ObservationNormalizer
participant DeclarationSelector
participant SccmClientDiscoveryResult
SCCMObservations->>ObservationNormalizer: raw observations
ObservationNormalizer->>DeclarationSelector: normalized declarations and coverage issues
DeclarationSelector->>SccmClientDiscoveryResult: capped declarations and diagnostics
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 |
|
@coderabbitai full review Please review exact head |
github-actions
Bot
added
bug
|
✅ Action performedFull review finished. |
|
Addressed both exact-head hosted CodeRabbit hardening suggestions from review |
|
@coderabbitai full review\n\nPlease perform a fresh hosted full 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 27 minutes. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
src-tauri/src/sccm/discovery.rs (3)
396-399: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the shadowing match binding for clarity.
The arm binds a new
root_is_validthat shadows the outer variable at line 340. The value is the same, so behavior is correct. A distinct name makes it explicit that this arm also matches(true, None), which is the unsupported-source case.♻️ Proposed rename
- (root_is_valid, catalog_basename) => { - let coverage_issue = coverage_issue_key(root_is_valid, catalog_basename.as_deref()); + (rejected_root_is_valid, catalog_basename) => { + let coverage_issue = + coverage_issue_key(rejected_root_is_valid, catalog_basename.as_deref()); add_coverage_issue_count(&mut coverage_issue_counts, coverage_issue, 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 `@src-tauri/src/sccm/discovery.rs` around lines 396 - 399, Rename the match arm’s root validity binding in the coverage issue handling around coverage_issue_key and add_coverage_issue_count to avoid shadowing the outer root_is_valid variable. Use a distinct name that clarifies this arm handles the unsupported (true, None) source case, while preserving the existing values passed to coverage_issue_key.
411-439: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the
nonecatalog identity into a shared constant.
"sccm-client-source:v1:none"appears at line 419 and line 433. The tests also assert the same literal. A single constant keeps the fallback identity aligned with thesccm-client-source:v1:prefix used bycatalog_entry_id.♻️ Proposed constant
+/// The privacy-safe catalog identity used when no validated catalog entry exists. +const NO_CATALOG_ENTRY_ID: &str = "sccm-client-source:v1:none"; + fn coverage_issue_key(root_is_valid: bool, catalog_basename: Option<&str>) -> CoverageIssueKey { @@ let catalog_entry_id = catalog_basename .map(catalog_entry_id) - .unwrap_or_else(|| "sccm-client-source:v1:none".to_owned()); + .unwrap_or_else(|| NO_CATALOG_ENTRY_ID.to_owned()); @@ CoverageIssueKey { - catalog_entry_id: "sccm-client-source:v1:none".to_owned(), + catalog_entry_id: NO_CATALOG_ENTRY_ID.to_owned(),🤖 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 `@src-tauri/src/sccm/discovery.rs` around lines 411 - 439, Extract "sccm-client-source:v1:none" into a shared constant near the catalog identity definitions, then use that constant in coverage_issue_key, declaration_limit_issue_key, and the related tests instead of repeating the literal. Keep the existing fallback identity unchanged and align the constant with the sccm-client-source:v1: prefix used by catalog_entry_id.
232-257: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueAdd a compile-time assertion that
MAX_SCCM_CLIENT_DISCOVERY_DECLARATIONS > 0. The existing observation-bound assertion does not enforce this requirement for the... - 1expressions.🤖 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 `@src-tauri/src/sccm/discovery.rs` around lines 232 - 257, Add a compile-time assertion near the existing SCCM discovery bounds assertions that validates MAX_SCCM_CLIENT_DISCOVERY_DECLARATIONS is greater than zero, ensuring the subtraction expressions in the selected-observation cap logic remain valid.src-tauri/tests/sccm_client_discovery.rs (1)
1123-1163: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a test where a capacity issue and a rejection issue occur together.
This test fills the declaration budget exactly, so the cap block in
discover_client_sourcesdoes not run. Testdiscovery_never_assigns_catalog_memberships_to_rejected_rotation_candidatesproduces two issue kinds but no overflow. No test asserts thatDeclarationLimitExceededcounts aggregate correctly into a map that already holdsInvalidProvenanceorUnsupportedkeys.Add one observation beyond the budget to this test. That covers the interaction between
add_coverage_issue_countcalls fromnormalize_observationsand from the cap block, and it pins the deterministic ordering of mixed issue kinds in the output vector.🤖 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 `@src-tauri/tests/sccm_client_discovery.rs` around lines 1123 - 1163, Extend discovery_retains_coverage_issues_past_the_declaration_cap_without_admitting_them_as_capture by adding one valid observation beyond MAX_SCCM_CLIENT_DISCOVERY_DECLARATIONS so the cap path executes alongside the existing InvalidProvenance issue. Update assertions to verify DeclarationLimitExceeded aggregates correctly with the existing coverage issue and assert the deterministic ordering and counts of both issue kinds in result.coverage_issues.
🤖 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 `@src-tauri/src/sccm/discovery.rs`:
- Around line 396-399: Rename the match arm’s root validity binding in the
coverage issue handling around coverage_issue_key and add_coverage_issue_count
to avoid shadowing the outer root_is_valid variable. Use a distinct name that
clarifies this arm handles the unsupported (true, None) source case, while
preserving the existing values passed to coverage_issue_key.
- Around line 411-439: Extract "sccm-client-source:v1:none" into a shared
constant near the catalog identity definitions, then use that constant in
coverage_issue_key, declaration_limit_issue_key, and the related tests instead
of repeating the literal. Keep the existing fallback identity unchanged and
align the constant with the sccm-client-source:v1: prefix used by
catalog_entry_id.
- Around line 232-257: Add a compile-time assertion near the existing SCCM
discovery bounds assertions that validates
MAX_SCCM_CLIENT_DISCOVERY_DECLARATIONS is greater than zero, ensuring the
subtraction expressions in the selected-observation cap logic remain valid.
In `@src-tauri/tests/sccm_client_discovery.rs`:
- Around line 1123-1163: Extend
discovery_retains_coverage_issues_past_the_declaration_cap_without_admitting_them_as_capture
by adding one valid observation beyond MAX_SCCM_CLIENT_DISCOVERY_DECLARATIONS so
the cap path executes alongside the existing InvalidProvenance issue. Update
assertions to verify DeclarationLimitExceeded aggregates correctly with the
existing coverage issue and assert the deterministic ordering and counts of both
issue kinds in result.coverage_issues.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6cdf1419-8054-46d1-9136-351a72d52f9d
📒 Files selected for processing (2)
src-tauri/src/sccm/discovery.rssrc-tauri/tests/sccm_client_discovery.rs
|
Hosted CodeRabbit review Addressed in
The requested mixed rejection-plus-declaration-cap test is unreachable under the existing defensive contract and was not fabricated. Independent exact-delta review returned GO/no P0-P3. Discovery 24/24, units 6/6, manifest 21/21, full parser, strict app/parser Clippy, wasm32, scoped Rustfmt/diff, and clean status passed. Local CodeRabbit exact |
|
@coderabbitai full review Please perform a fresh hosted full 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 14 minutes. |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src-tauri/src/sccm/discovery.rs (2)
460-533: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueGuard the identity fields against future key growth.
CoverageIssueKeydistinguishes issues by five fields, butcoverage_issue_idhashes only four of them.logical_artifact_idsis not part of the payload. Today every key builder setslogical_artifact_ids: Vec::new(), so no two distinct keys can collide. If a later change populates that vector for some issue category, two distinct coverage issues will share oneartifact_idand callers that key onartifact_idwill merge them silently.Add a
debug_assert!(logical_artifact_ids.is_empty())incoverage_issue_from_key, or include the field in the hashed payload.♻️ Suggested guard
} = key; + debug_assert!( + logical_artifact_ids.is_empty(), + "coverage identity does not hash logical artifact IDs; populating them would alias issues" + ); let artifact_id = coverage_issue_id( &catalog_entry_id, rotation_category, state, omitted_declaration_state, );🤖 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 `@src-tauri/src/sccm/discovery.rs` around lines 460 - 533, Add a debug_assert!(logical_artifact_ids.is_empty()) in coverage_issue_from_key immediately after destructuring CoverageIssueKey, before calling coverage_issue_id, to guard the current identity contract without changing the existing artifact ID format.
246-259: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the terminal-retention invariant.
The arithmetic here is correct. With
N = selected.len()andM = MAX_SCCM_CLIENT_DISCOVERY_DECLARATIONS, the guard givesN >= M + 1, so the slice[M - 1..]over the post-poplengthN - 1is always in bounds and yields exactlyN - Momitted entries. The compile-time assertion at line 444 prevents theM - 1underflow.The non-obvious part is that the retained output is not a contiguous prefix: the code keeps the first
M - 1entries plus the deterministic terminal entry, and drops from the middle. Add a short comment stating that invariant so a future edit does not replace this block with a plaintruncate.♻️ Suggested comment
+ // The bounded output keeps the first MAX - 1 selected observations plus the + // deterministic terminal observation, so the omitted range is the middle + // slice. Do not replace this with a plain prefix truncation. if selected.len() > MAX_SCCM_CLIENT_DISCOVERY_DECLARATIONS { let terminal = selected .pop() .expect("an over-cap selection has a terminal observation");🤖 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 `@src-tauri/src/sccm/discovery.rs` around lines 246 - 259, Add a concise comment immediately before the over-cap selection block explaining that it intentionally retains the first MAX_SCCM_CLIENT_DISCOVERY_DECLARATIONS - 1 entries plus the deterministic terminal observation, omitting entries from the middle; preserve the existing pop, coverage counting, truncate, and terminal push behavior.src-tauri/tests/sccm_client_discovery.rs (1)
99-105: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse the new helper in
expected_evidence_identity.
expected_evidence_identityat lines 112-115 still builds the same catalog-entry string inline. Call the new helper there so the expected identity formula has one definition in this file.♻️ Suggested change
- let catalog_entry_id = format!( - "sccm-client-source:v1:sha256:{}", - sha256(canonical_basename) - ); + let catalog_entry_id = expected_catalog_entry_id(canonical_basename);🤖 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 `@src-tauri/tests/sccm_client_discovery.rs` around lines 99 - 105, Update expected_evidence_identity to call expected_catalog_entry_id for the catalog-entry component instead of constructing the SHA-256 string inline, leaving the rest of the expected identity logic unchanged.
🤖 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 `@src-tauri/src/sccm/discovery.rs`:
- Around line 460-533: Add a debug_assert!(logical_artifact_ids.is_empty()) in
coverage_issue_from_key immediately after destructuring CoverageIssueKey, before
calling coverage_issue_id, to guard the current identity contract without
changing the existing artifact ID format.
- Around line 246-259: Add a concise comment immediately before the over-cap
selection block explaining that it intentionally retains the first
MAX_SCCM_CLIENT_DISCOVERY_DECLARATIONS - 1 entries plus the deterministic
terminal observation, omitting entries from the middle; preserve the existing
pop, coverage counting, truncate, and terminal push behavior.
In `@src-tauri/tests/sccm_client_discovery.rs`:
- Around line 99-105: Update expected_evidence_identity to call
expected_catalog_entry_id for the catalog-entry component instead of
constructing the SHA-256 string inline, leaving the rest of the expected
identity logic unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 32dc194c-5c01-4e1c-8a07-0fdadc902e22
📒 Files selected for processing (2)
src-tauri/src/sccm/discovery.rssrc-tauri/tests/sccm_client_discovery.rs
|
Addressed all three exact-head hosted CodeRabbit findings from
The release assertion does not change valid output or IDs because every admitted coverage-key constructor already supplies an empty logical-artifact vector. Controller exact-delta review found no P0-P3 issue; discovery 24/24 and discovery units 6/6 passed; the implementing owner also passed manifest 21/21, full parser, strict app/parser Clippy, wasm32, TypeScript, scoped Rustfmt/diff, and clean status. Local CodeRabbit exact |
|
@coderabbitai full review Please perform a fresh hosted full review of exact head |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src-tauri/tests/sccm_client_discovery.rs (1)
766-797: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winPin the disposition conflict with a same-state case.
The two observations differ in
stateand in disposition, so the assertion passes even ifdispositionwere removed fromObservationFacts. Add a case where both observations carry the same state but one classifies as accepted and the other as rejected. That case exercises thedispositionfield alone.💚 Proposed additional case
#[test] fn discovery_conflicts_accepted_and_rejected_dispositions_with_one_shared_state() { let input = SccmClientDiscoveryInput { max_found_fragments_per_source: 8, observations: vec![ observation( ROOT_A, "AppEnforce.log", SccmRotation::Current, SccmClientDiscoveryObservationState::Found, ), observation( ROOT_A, "AppEnforce.log", unsupported_rotation(".backup"), SccmClientDiscoveryObservationState::Found, ), ], }; assert_eq!( discover_client_sources(&input) .expect_err("one raw observation cannot be both accepted and rejected"), SccmClientDiscoveryError::ConflictingObservation ); }🤖 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 `@src-tauri/tests/sccm_client_discovery.rs` around lines 766 - 797, Add a same-state disposition-conflict test alongside discovery_rejects_accepted_and_rejected_facts_for_one_raw_physical_observation, using Found for both observations while keeping one accepted and one rejected rotation. Assert discover_client_sources returns SccmClientDiscoveryError::ConflictingObservation, ensuring the conflict is detected through disposition independently of state.src-tauri/src/sccm/discovery.rs (1)
938-950: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract a local
unsupported_rotationhelper.The inline
SccmRotation::Unknownconstruction repeats the shape already used by the integration test helper. A small local helper keeps the test body focused on the assertion.♻️ Proposed refactor
+ fn unsupported_rotation(suffix: &str) -> SccmRotation { + SccmRotation::Unknown(cmtraceopen_parser::sccm::SccmUnknownRotation { + kind: "filenameSuffix".to_owned(), + value: Some(serde_json::Value::String(suffix.to_owned())), + }) + } + #[test] fn rejected_duplicate_boundary_has_bounded_classification_and_consistency_work() { let observations = (0..MAX_SCCM_CLIENT_DISCOVERY_OBSERVATIONS) .map(|_| SccmClientDiscoveryObservation { root_handle: "unvalidated-root".to_owned(), basename: "Unrelated.log.backup".to_owned(), - rotation: SccmRotation::Unknown(cmtraceopen_parser::sccm::SccmUnknownRotation { - kind: "filenameSuffix".to_owned(), - value: Some(serde_json::Value::String(".backup".to_owned())), - }), + rotation: unsupported_rotation(".backup"), state: SccmClientDiscoveryObservationState::Found, }) .collect();🤖 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 `@src-tauri/src/sccm/discovery.rs` around lines 938 - 950, Extract the repeated SccmRotation::Unknown construction from rejected_duplicate_boundary_has_bounded_classification_and_consistency_work into a local unsupported_rotation helper, matching the existing integration-test helper’s shape, and use that helper when initializing rotation so the test remains focused on its assertion.
🤖 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 `@src-tauri/src/sccm/discovery.rs`:
- Around line 938-950: Extract the repeated SccmRotation::Unknown construction
from rejected_duplicate_boundary_has_bounded_classification_and_consistency_work
into a local unsupported_rotation helper, matching the existing integration-test
helper’s shape, and use that helper when initializing rotation so the test
remains focused on its assertion.
In `@src-tauri/tests/sccm_client_discovery.rs`:
- Around line 766-797: Add a same-state disposition-conflict test alongside
discovery_rejects_accepted_and_rejected_facts_for_one_raw_physical_observation,
using Found for both observations while keeping one accepted and one rejected
rotation. Assert discover_client_sources returns
SccmClientDiscoveryError::ConflictingObservation, ensuring the conflict is
detected through disposition independently of state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e02ff8d0-7bb0-49c8-8841-e4b6691094db
📒 Files selected for processing (2)
src-tauri/src/sccm/discovery.rssrc-tauri/tests/sccm_client_discovery.rs
|
Closed both exact-head CodeRabbit test-review items in c2454d4: a new same-state accepted-versus-rejected disposition regression proves the conflict is fail-closed, order-independent, and privacy-safe; repeated unsupported-rotation unit setup now uses one helper. No production behavior changed. Fresh discovery 25/25, discovery units 6/6, manifest 21/21, full parser, strict parser/app Clippy, wasm32, TypeScript, scoped Rustfmt/diff, and clean status passed. Local CodeRabbit over the exact test-only delta returned zero findings. |
|
@coderabbitai full review Please perform a fresh hosted full review of exact head c2454d4 against base c622c9e. The two final test-review items are addressed without changing production behavior. |
|
✅ Action performedFull review finished. |
Preserve rejected and omitted SCCM client discovery coverage deterministically while keeping declarations bounded and privacy-safe. Reviewed with exact-head Copilot and CodeRabbit; focused, parser, strict Clippy, wasm32, TypeScript, Windows ESP, and cross-platform packaging gates passed.
Follow-up to merged #444, based on its exact integration commit
c622c9ec.This 23-commit correction keeps SCCM client discovery deterministic and privacy-safe when supplied observations are rejected or declaration capacity is reached:
Skipped/NotFound/Foundstates; andThe first 16 commits preserve the reviewed RED/GREEN sequence.
d3c096f1is a test-only local-CodeRabbit fix for derived-DebugWindows-path privacy assertions. Hosted Copilot then found that coverage identity payloads used printable\\0separators instead of the SCCM identity convention of NUL boundaries. RED7589ccddpins both omitted-state branches; GREEN207ac2f7changes only those payload strings to actual NUL separators. Hosted CodeRabbit then requested invariant hardenings:b362e89ereuses the canonical catalog-ID helper in the separator regression and proves the observation ceiling fits theu16coverage counter;f1f126bbremoves a shadowed binding, centralizes the no-catalog identity, and proves the declaration ceiling is nonzero;f5ab362aenforces empty workflow membership before the current four-field coverage-ID hash, documents deterministic terminal retention, and reuses the test catalog-ID helper. These commits do not change valid runtime output or IDs.c2454d4aadds the hosted-requested same-state accepted/rejected disposition regression and consolidates repeated unsupported-rotation test setup; it changes no production behavior.CodeRabbit's requested mixed rejection-plus-declaration-cap regression is mathematically unreachable under the defensive contract and was not fabricated: the cap branch requires all 4,097 permitted observations to be accepted/selected, leaving no input slot for a rejected observation. Existing executable guards pin both reachable edges: 4,096 valid plus one rejected yields only rejection coverage, while 4,097 selected valid observations yields declaration-cap coverage.
Exact scope remains
src-tauri/src/sccm/discovery.rsandsrc-tauri/tests/sccm_client_discovery.rsonly.Verification at the corrected candidate:
--all-targets, strict app/parser Clippy, and parser wasm32;git diff --check, clean branch status, and TypeScripttsc --noEmit;c622c9ec..d3c096f1: zero findings; exact hosted-fix deltad3c096f1..207ac2f7: zero findings; exact hardening deltas207ac2f7..b362e89e,b362e89e..f1f126bb, andf1f126bb..f5ab362aand final test-only deltaf5ab362a..c2454d4a: zero findings;0c4cf545; scoped privacy-fix review: GO atd3c096f1; exact hosted-separator fix review: GO at207ac2f7; exact hardening reviews: GO atb362e89e,f1f126bb, controller GO atf5ab362a, and controller plus local CodeRabbit review of the test-onlyc2454d4adelta, all with no P0-P3 findings.Repository-wide
cargo fmt --check --allretains inherited drift at the integration base in unrelated files; both changed Rust files pass scoped Rustfmt.No native Windows collection, live SCCM validation, capture, workflow diagnosis, correlation, or production acceptance is claimed.
This is intentionally a draft. Fresh hosted exact-head CodeRabbit, GitHub Copilot, all CI including Windows, zero unresolved actionable threads, and a final live base/head merge guard are required before merge.
Refs #319
Refs #317
Summary by CodeRabbit
New Features
Bug Fixes