feat(sccm): add bounded client discovery normalization - #444
Conversation
📝 WalkthroughWalkthroughAdds a bounded SCCM client-source discovery API. It normalizes and deduplicates observations, rejects conflicts, applies deterministic per-source and global caps, and emits physical or marker declarations with evidence and fingerprints. Tests cover limits, ordering, identity safety, and malformed inputs. ChangesSCCM client discovery
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant discover_client_sources
participant ObservationNormalization
participant DeclarationConstruction
Caller->>discover_client_sources: Submit SCCM observations
discover_client_sources->>ObservationNormalization: Validate, canonicalize, and deduplicate
ObservationNormalization-->>discover_client_sources: Ordered candidates
discover_client_sources->>DeclarationConstruction: Apply caps and build declarations
DeclarationConstruction-->>discover_client_sources: Bounded declarations and result
discover_client_sources-->>Caller: Return discovery result or error
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
github-actions
Bot
added
enhancement
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Exact-head review request for @coderabbitai review Please review the complete committed range and generated merge ref. Focus on pre-normalization CPU/allocation bounds, exact 4,096/4,097 behavior, deterministic conflict handling, duplicate identity safety, source fairness, opaque-root privacy, malformed input fail-close behavior, wasm32 parser purity, and the absence of capture/I/O or generic manifest-status changes. Report actionable findings against this head only; do not push code changes. This is discovery normalization over already-observed metadata. It does not enumerate paths, capture or publish evidence, or claim native/live Windows SCCM acceptance. The PR remains draft until exact-head hosted CI, both review systems, unresolved-thread checks, and a final merge guard pass. |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
base='431909709ae3e66bb4c236647af92bd5312bcc90'
head='c778b3cd66d3969eb5f8f87c407f8964f7d520e4'
printf '%s\n' '== Exact revisions =='
git rev-parse "$base" "$head"
printf '%s\n' '== Ancestry and merge base =='
git merge-base "$base" "$head"
git merge-base --is-ancestor "$base" "$head" && echo 'base-is-ancestor=yes'
printf '%s\n' '== Changed paths =='
git diff --name-status "$base...$head"
printf '%s\n' '== Diff stat =='
git diff --stat "$base...$head"
printf '%s\n' '== Candidate Rust files =='
git diff --name-only --diff-filter=ACMR "$base...$head" | grep -E '\.rs$' || true
printf '%s\n' '== AST outlines for changed Rust files =='
git diff --name-only --diff-filter=ACMR "$base...$head" | grep -E '\.rs$' | while IFS= read -r file; do
echo "--- $file"
ast-grep outline "$file" --items all --view expanded || true
done
printf '%s\n' '== Full committed diff =='
git diff --find-renames --find-copies --no-ext-diff "$base...$head"Length of output: 50016 🏁 Script executed: #!/bin/bash
set -euo pipefail
base='431909709ae3e66bb4c236647af92bd5312bcc90'
head='c778b3cd66d3969eb5f8f87c407f8964f7d520e4'
printf '%s\n' '== Locate contract and parser rotation definitions =='
fd -t f 'contract\.rs|sccm.*\.rs' src-tauri/src src-tauri/tests . | sort
printf '%s\n' '== SCCM contract outline =='
ast-grep outline src-tauri/src/sccm/contract.rs --items all --view expanded
printf '%s\n' '== Relevant SCCM contract implementations =='
rg -n -A45 -B5 \
'fn (root_handle_digest|source_identity_digest|canonical_client_source|rotation_segment|rotation_order|expected_physical_artifact_id|expected_marker_artifact_id|logical_artifact_ids_for_basename|catalog_entry_id)\b|enum SccmRotation\b' \
src-tauri/src/sccm/contract.rs src-tauri 2>/dev/null || true
printf '%s\n' '== Parser SCCM rotation declarations and uses =='
rg -n -A30 -B8 'enum SccmRotation\b|SccmRotation::(Unknown|Timestamped|Numbered|LoUnderscore|Current)' \
--glob '*.rs' . | head -n 900
printf '%s\n' '== Changed-range prohibited-scope indicators =='
git diff --unified=3 "$base...$head" -- \
'src-tauri/**/*.rs' 'src-tauri/Cargo.toml' \
| rg -n -i -C2 \
'\b(std::fs|tokio::fs|File::|OpenOptions|read_dir|write_all|remove_file|create_dir|Command::|reqwest|ureq|tauri::|ArtifactStatus|ParserKind|capture|publish|evidence file|windows|winapi)\b' \
|| true
printf '%s\n' '== Direct imports and calls in discovery implementation =='
rg -n -C2 \
'^(use |pub use )|\b(root_handle_digest|canonical_client_source|rotation_segment|source_identity_digest|candidate_from_observation|declaration_from_candidate|normalize_observations|selection_state)\s*\(' \
src-tauri/src/sccm/discovery.rsLength of output: 121756 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '== Find parser source files that define SCCM rotations and classification =='
rg -n -l --glob '*.rs' \
'enum SccmRotation\b|fn classify_artifact_name\b|struct .*Artifact.*Name|supported_for_diagnosis' \
crates/cmtraceopen-parser src-tauri/src | sort
printf '%s\n' '== AST map of parser SCCM source files =='
fd -t f -e rs . crates/cmtraceopen-parser/src | while IFS= read -r file; do
if rg -q 'enum SccmRotation\b|fn classify_artifact_name\b' "$file"; then
echo "--- $file"
ast-grep outline "$file" --items all --view expanded
fi
done
printf '%s\n' '== Exact definitions and classifier call path =='
rg -n -A90 -B10 --glob '*.rs' \
'enum SccmRotation\b|fn classify_artifact_name\b|fn .*rotation|supported_for_diagnosis' \
crates/cmtraceopen-parser/src src-tauri/src/sccm/contract.rs
printf '%s\n' '== Exact-range final integrity checks =='
base='431909709ae3e66bb4c236647af92bd5312bcc90'
head='c778b3cd66d3969eb5f8f87c407f8964f7d520e4'
git diff --check "$base...$head"
printf 'changed-files='
git diff --name-only "$base...$head" | wc -l
printf 'merge-base='
git merge-base "$base" "$head"Length of output: 211980
I found no actionable findings in the committed range. The range contains four changed files. The defensive 4,097-observation rejection occurs before normalization and construction. The output remains bounded to 4,096 declarations. Conflict detection occurs before selection. Duplicate coalescing, deterministic ordering, per-source capping, opaque root validation, and cross-root identity derivation are consistent with the stated contract. The changed range contains no detected capture, I/O, generic The sandbox did not provide a generated merge-ref revision. I reviewed the exact requested base-to-head range. I did not execute compilation or tests in this environment. ✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (6)
src-tauri/src/sccm/discovery.rs (4)
543-609: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for a zero per-source cap.
No test supplies
max_found_fragments_per_source: 0. With that value,selection_statenever takes the increment branch, so the first Found observation for each source becomesCappedand every later fragment is dropped. That behavior is reasonable, but it is unverified and it is the boundary a caller is most likely to reach by accident.🤖 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 543 - 609, Add a test covering discover_client_sources with max_found_fragments_per_source set to 0. Verify that the first Found observation for each source is reported as Capped and subsequent fragments are omitted, while preserving the existing declaration and construction limits.
339-346: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse the shared hex helper.
contract::sha256_bytesalready performs this digest-and-hex-encode step (src-tauri/src/sccm/contract.rs:394-399) and ispub(crate). Call it here to keep one hex encoding in the SCCM module.♻️ Proposed deduplication
- let digest = Sha256::digest(value.as_bytes()); - format!( - "sccm-evidence:v1:sha256:{}", - digest - .iter() - .map(|byte| format!("{byte:02x}")) - .collect::<String>() - ) + format!("sccm-evidence:v1:sha256:{}", sha256_bytes(value.as_bytes()))Then drop the now-unused
sha2import and addsha256_bytesto thesuper::contractimport 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 `@src-tauri/src/sccm/discovery.rs` around lines 339 - 346, Update the evidence formatting logic to call the shared `contract::sha256_bytes` helper for digest-and-hex encoding instead of using `Sha256` directly. Add `sha256_bytes` to the `super::contract` imports and remove the now-unused `sha2` import, preserving the existing `sccm-evidence:v1:sha256:` output format.
349-365: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winPrecompute the logical artifact IDs instead of recomputing them per comparison.
compare_observation_ordercallslogical_artifact_ids_for_basenametwice on every invocation. That helper scansdeclared_client_source_groups(), allocates aVec<String>, and sorts it (src-tauri/src/sccm/contract.rs:419-432). The comparator runs from two places: the tie-break at Line 192 and thesort_byat Line 199. With the 4,097-observation admission bound, the sort alone performs about 49,000 comparisons, so this allocates roughly 100,000 vectors of owned strings per call.
NormalizedObservationalready cachescanonical_basename. Cache the derived IDs next to it innormalize_observation, then compare the cached field.candidate_from_observationcan also reuse the cached value instead of recomputing it at Line 254.♻️ Proposed caching of the ordering key
struct NormalizedObservation<'a> { observation: &'a SccmClientDiscoveryObservation, canonical_basename: String, + logical_artifact_ids: Vec<String>, }root_handle_digest(&observation.root_handle)?; let canonical_basename = canonical_client_source(&observation.basename, &observation.rotation)?; Some(NormalizedObservation { observation, + logical_artifact_ids: logical_artifact_ids_for_basename(&canonical_basename), canonical_basename, })fn compare_observation_order( left: &NormalizedObservation<'_>, right: &NormalizedObservation<'_>, ) -> Ordering { - logical_artifact_ids_for_basename(&left.canonical_basename) - .cmp(&logical_artifact_ids_for_basename( - &right.canonical_basename, - )) + left.logical_artifact_ids + .cmp(&right.logical_artifact_ids) .then_with(|| {🤖 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 349 - 365, Cache the result of logical_artifact_ids_for_basename while constructing each NormalizedObservation in normalize_observation, storing it alongside canonical_basename. Update compare_observation_order to compare the cached IDs directly, and update candidate_from_observation to reuse the cached value instead of calling the helper again.
132-132: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winReserve capacity from the input size, not the global cap.
Vec::with_capacity(MAX_SCCM_CLIENT_DISCOVERY_DECLARATIONS)allocates 4,096 declaration slots on every call.SccmClientDiscoveryDeclarationholds seven owned fields, so this reserves roughly a megabyte even when the caller supplies one observation. The output can never exceed the observation count, so bound the reservation by both values.♻️ Proposed capacity bound
- let mut declarations = Vec::with_capacity(MAX_SCCM_CLIENT_DISCOVERY_DECLARATIONS); + let mut declarations = Vec::with_capacity( + input + .observations + .len() + .min(MAX_SCCM_CLIENT_DISCOVERY_DECLARATIONS), + );🤖 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` at line 132, Update the declarations initialization in the SCCM discovery flow to reserve capacity based on the input observation count, capped at MAX_SCCM_CLIENT_DISCOVERY_DECLARATIONS. Keep the existing global limit for output enforcement while ensuring small inputs do not preallocate the full cap.src-tauri/tests/sccm_client_discovery.rs (2)
482-490: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the error variant, not only that an error occurs.
This test checks that both orders return an error, but it never checks which error.
SccmClientDiscoveryError::ObservationLimitExceededwould satisfy the assertion equally. The sibling tests at Lines 452 and 547 pin the variant. Pin it here as well so the test cannot pass for the wrong reason.💚 Proposed assertion
let error = discover_client_sources(&input) .expect_err("canonical aliases with conflicting state fail closed"); + assert_eq!(error, SccmClientDiscoveryError::ConflictingObservation); let mut reversed = input;🤖 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 482 - 490, Update the test around discover_client_sources to assert that both the original and reversed observation orders return SccmClientDiscoveryError::ConflictingCanonicalAlias, rather than only asserting that an error occurs. Preserve the existing order-independence check while pinning the expected error variant.
555-579: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlso cover the unsupported-basename skip path.
This test covers the malformed-root skip at Line 208 of
src-tauri/src/sccm/discovery.rs.normalize_observationhas a second silent skip at Line 209, whencanonical_client_sourcerejects a basename that is not a supported client source. No test covers that path. Add an observation with a valid root and an unsupported basename, and assert that discovery skips it and still returns the valid declaration.🤖 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 555 - 579, Extend discovery_skips_supported_observations_with_malformed_root_handles to include an observation with a valid root handle and an unsupported basename. Keep the existing valid observation, then assert discovery succeeds with only the valid declaration, confirming normalize_observation skips unsupported basenames without affecting supported sources.
🤖 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 543-609: Add a test covering discover_client_sources with
max_found_fragments_per_source set to 0. Verify that the first Found observation
for each source is reported as Capped and subsequent fragments are omitted,
while preserving the existing declaration and construction limits.
- Around line 339-346: Update the evidence formatting logic to call the shared
`contract::sha256_bytes` helper for digest-and-hex encoding instead of using
`Sha256` directly. Add `sha256_bytes` to the `super::contract` imports and
remove the now-unused `sha2` import, preserving the existing
`sccm-evidence:v1:sha256:` output format.
- Around line 349-365: Cache the result of logical_artifact_ids_for_basename
while constructing each NormalizedObservation in normalize_observation, storing
it alongside canonical_basename. Update compare_observation_order to compare the
cached IDs directly, and update candidate_from_observation to reuse the cached
value instead of calling the helper again.
- Line 132: Update the declarations initialization in the SCCM discovery flow to
reserve capacity based on the input observation count, capped at
MAX_SCCM_CLIENT_DISCOVERY_DECLARATIONS. Keep the existing global limit for
output enforcement while ensuring small inputs do not preallocate the full cap.
In `@src-tauri/tests/sccm_client_discovery.rs`:
- Around line 482-490: Update the test around discover_client_sources to assert
that both the original and reversed observation orders return
SccmClientDiscoveryError::ConflictingCanonicalAlias, rather than only asserting
that an error occurs. Preserve the existing order-independence check while
pinning the expected error variant.
- Around line 555-579: Extend
discovery_skips_supported_observations_with_malformed_root_handles to include an
observation with a valid root handle and an unsupported basename. Keep the
existing valid observation, then assert discovery succeeds with only the valid
declaration, confirming normalize_observation skips unsupported basenames
without affecting supported sources.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ec24dd18-5d8f-44a2-b58a-4fc5319d0870
📒 Files selected for processing (4)
src-tauri/Cargo.tomlsrc-tauri/src/sccm/discovery.rssrc-tauri/src/sccm/mod.rssrc-tauri/tests/sccm_client_discovery.rs
There was a problem hiding this comment.
Pull request overview
Adds a native-side SCCM client discovery normalizer that turns bounded, already-observed client source candidates into deterministic manifest-style declarations, including duplicate coalescing, conflict fail-close behavior, and explicit “capped” gap signaling.
Changes:
- Introduces
src-tauri/src/sccm/discovery.rswith bounded normalization + deterministic ordering + per-source/global caps. - Exposes the discovery API via
src-tauri/src/sccm/mod.rs. - Adds a feature-gated integration test suite (
sccm_client_discovery) and wires it intosrc-tauri/Cargo.toml.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src-tauri/tests/sccm_client_discovery.rs | New integration tests covering cap boundaries, dedup/conflict behavior, ordering determinism, and ID stability expectations. |
| src-tauri/src/sccm/mod.rs | Registers and re-exports the new discovery module in the SCCM surface area. |
| src-tauri/src/sccm/discovery.rs | Implements the bounded discovery normalization pipeline and related types/constants. |
| src-tauri/Cargo.toml | Adds the new feature-gated test target for SCCM discovery. |
|
Exact-head re-review request for @coderabbitai review Please review the complete committed range and generated merge ref, including the fixes for canonical physical/evidence identities and bounded logical-ID work. Focus on exact 4,096/4,097 behavior, deterministic conflict handling, case-equivalent current/ Focused RED: |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
base='431909709ae3e66bb4c236647af92bd5312bcc90'
head='de0957d48905d453aa0f0accb03d901a0363d5af'
echo '== exact revisions and ancestry =='
git rev-parse "$base" "$head"
git merge-base "$base" "$head"
git merge-base --is-ancestor "$base" "$head" && echo 'base-is-ancestor=yes'
echo '== merge-ref availability =='
git show-ref --head | rg -n -i 'merge|pull|444' || true
echo '== changed paths and integrity =='
git diff --name-status "$base...$head"
git diff --stat "$base...$head"
git diff --check "$base...$head"
echo '== changed Rust outlines =='
git diff --name-only --diff-filter=ACMR "$base...$head" -- '*.rs' | while IFS= read -r file; do
echo "--- $file"
ast-grep outline "$file" --items all --view expanded || true
done
echo '== discovery definitions and normalization flow =='
ast-grep outline src-tauri/src/sccm/discovery.rs --items all --view expanded
rg -n -A70 -B12 --glob '*.rs' \
'pub fn discover_client_sources\b|fn normalize_observations\b|fn candidate_from_observation\b|fn declaration_from_candidate\b|fn selection_state\b|MAX_SCCM_CLIENT_DISCOVERY_(DECLARATIONS|OBSERVATIONS)\b|logical_artifact_ids|root_handle_digest|source_identity_digest|canonical_client_source|rotation_(segment|order)' \
src-tauri/src/sccm/discovery.rs src-tauri/src/sccm/contract.rs crates 2>/dev/null || true
echo '== focused tests =='
ast-grep outline src-tauri/tests/sccm_client_discovery.rs --items all --view expanded
rg -n -A35 -B8 --glob '*.rs' \
'409[67]|case|alias|[Ll][Oo]_|Current|Numbered|conflict|fair|opaque|malformed|logical.*id|physical.*id|evidence.*id|determin' \
src-tauri/tests/sccm_client_discovery.rs src-tauri/src/sccm/discovery.rs
echo '== prohibited scope indicators in exact changed range =='
git diff --unified=4 "$base...$head" -- 'src-tauri/**/*.rs' 'src-tauri/Cargo.toml' |
rg -n -i -C3 \
'\b(std::fs|tokio::fs|File::|OpenOptions|read_dir|write_all|remove_file|create_dir|Command::|reqwest|ureq|tauri::|ArtifactStatus|ParserKind|capture|publish|windows|winapi)\b' || true
echo '== exact committed diff =='
git diff --find-renames --find-copies --no-ext-diff "$base...$head" -- \
src-tauri/Cargo.toml \
src-tauri/src/sccm/discovery.rs \
src-tauri/src/sccm/mod.rs \
src-tauri/tests/sccm_client_discovery.rsLength of output: 134713 🏁 Script executed: #!/bin/bash
set -euo pipefail
base='431909709ae3e66bb4c236647af92bd5312bcc90'
head='de0957d48905d453aa0f0accb03d901a0363d5af'
echo '== exact revision metadata =='
git rev-parse "$base" "$head"
echo -n 'merge-base='
git merge-base "$base" "$head"
git merge-base --is-ancestor "$base" "$head" && echo 'base-is-ancestor=yes'
echo '== possible generated merge refs =='
git show-ref --head | rg -n -i 'refs/(pull|merge)|pull/444|merge' || true
echo '== contract validation functions and comments =='
ast-grep outline src-tauri/src/sccm/contract.rs --items all --view expanded
rg -n -A80 -B15 --glob '*.rs' \
'fn root_handle_digest\b|fn canonical_client_source\b|fn logical_artifact_ids_for_basename\b|fn source_identity_digest\b|invalid root|malformed root|unsupported basename|fail.?close|skip' \
src-tauri crates tests 2>/dev/null || true
echo '== malformed-input contract references in repository text =='
rg -n -i -C4 \
'sccm.{0,80}(malformed|invalid|unsupported|fail.?close|root handle)|\
(malformed|invalid|unsupported|fail.?close).{0,80}sccm' \
--glob '!target/**' --glob '!node_modules/**' .
echo '== complete discovery range around validation and errors =='
git show "$head:src-tauri/src/sccm/discovery.rs" | nl -ba | sed -n '80,230p'
echo '== complete integration test around malformed input =='
git show "$head:src-tauri/tests/sccm_client_discovery.rs" | nl -ba | sed -n '630,710p'Length of output: 202105 🏁 Script executed: #!/bin/bash
set -euo pipefail
base='431909709ae3e66bb4c236647af92bd5312bcc90'
head='de0957d48905d453aa0f0accb03d901a0363d5af'
echo '== revisions =='
printf 'base='; git rev-parse "$base"
printf 'head='; git rev-parse "$head"
printf 'merge-base='; git merge-base "$base" "$head"
git merge-base --is-ancestor "$base" "$head" && echo 'base-is-ancestor=yes'
echo '== merge refs =='
git for-each-ref --format='%(refname) %(objectname)' |
rg -i 'pull/444|merge' || echo 'none-found'
echo '== changed files =='
git diff --name-only "$base...$head"
echo '== diff check =='
git diff --check "$base...$head" && echo 'passed'
echo '== prohibited APIs in changed implementation =='
git diff --unified=0 "$base...$head" -- src-tauri/src/sccm/discovery.rs src-tauri/src/sccm/mod.rs |
rg -n -i \
'\b(std::fs|tokio::fs|File::|OpenOptions|read_dir|write_all|remove_file|create_dir|Command::|reqwest|ureq|tauri::|ArtifactStatus|ParserKind|winapi|windows)\b' \
|| echo 'none-found'
echo '== malformed observation handling =='
git show "$head:src-tauri/src/sccm/discovery.rs" |
nl -ba | sed -n '122,160p'
git show "$head:src-tauri/tests/sccm_client_discovery.rs" |
nl -ba | sed -n '662,691p'Length of output: 2005
I found one actionable finding.
The exact base is an ancestor of the requested head. No generated merge ref was available in the sandbox. I reviewed the requested committed range only. I did not run compilation or tests in this environment. ✅ Action performedReview finished.
|
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)
src-tauri/src/sccm/discovery.rs (1)
209-216: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReject malformed observations instead of silently omitting them.
Line 211 returns
Nonefor an invalid root handle or unsupported basename. The caller then receivesOkwith incomplete discovery results and cannot detect the invalid input. Return a privacy-safe validation error and abort the batch.
src-tauri/src/sccm/discovery.rs#L209-L216: Return a sanitized malformed-observation error instead ofNone.src-tauri/tests/sccm_client_discovery.rs#L662-L686: Require that malformed roots and unsupported basenames return that error. Do not expect the valid observation to succeed alone.🤖 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 209 - 216, Update normalize_observation in src-tauri/src/sccm/discovery.rs (lines 209-216) to return a sanitized privacy-safe malformed-observation error for invalid root handles or unsupported basenames, propagating it so the batch aborts instead of returning None and producing incomplete results. Update the malformed-observation assertions in src-tauri/tests/sccm_client_discovery.rs (lines 662-686) to require that both malformed roots and unsupported basenames return this error, without expecting the valid observation to succeed independently.
🤖 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 `@src-tauri/src/sccm/discovery.rs`:
- Around line 209-216: Update normalize_observation in
src-tauri/src/sccm/discovery.rs (lines 209-216) to return a sanitized
privacy-safe malformed-observation error for invalid root handles or unsupported
basenames, propagating it so the batch aborts instead of returning None and
producing incomplete results. Update the malformed-observation assertions in
src-tauri/tests/sccm_client_discovery.rs (lines 662-686) to require that both
malformed roots and unsupported basenames return this error, without expecting
the valid observation to succeed independently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3c333527-9f0d-485e-88a7-a85bec5a7961
📒 Files selected for processing (2)
src-tauri/src/sccm/discovery.rssrc-tauri/tests/sccm_client_discovery.rs
|
The fresh exact-head CodeRabbit observation is accepted as exposing a real coverage-integrity gap, but the suggested whole-batch abort is not the contract-correct repair. At The correction must distinguish:
A fresh isolated TDD worktree is active from exact |
* test(sccm): define bounded client discovery contract * test(sccm): pin discovery budget and privacy boundaries * feat(sccm): normalize bounded client discovery * test(sccm): expose discovery bound and conflict gaps * test(sccm): isolate discovery construction probes * test(sccm): pin bounded discovery source fairness * fix(sccm): bound deterministic client discovery * test(sccm): expose late discovery conflicts * fix(sccm): reject all conflicting discovery observations * test(sccm): preserve malformed root discovery skip * fix(sccm): skip malformed discovery root handles * perf(sccm): short-circuit discovery conflict checks * test(sccm): pin conflict error privacy * style(sccm): satisfy bounded discovery lint * test(sccm): expose discovery work bound * fix(sccm): bound client discovery normalization work * test(sccm): expose discovery review regressions * fix(sccm): stabilize discovery normalization identities
Scope
Implements the bounded native-side SCCM client discovery normalizer for the accepted additive client-intake manifest contract.
.lo_, and numbered rotation identities;This slice does not enumerate Windows paths, read or write artifacts, capture evidence, mutate generic collection-manifest/
ArtifactStatussemantics, addParserKind::Sccm, perform workflow diagnosis/correlation, or claim live Windows/SCCM validation.Dependency state
431909709ae3e66bb4c236647af92bd5312bcc90(native manifest reader merged first).de0957d48905d453aa0f0accb03d901a0363d5af.Test matrix
.lo_, and numbered rotations;Verification
cargo test --locked -p app --test sccm_client_discovery— 12 passed;cargo test --locked -p app --test sccm_client_native_manifest— 21 passed;cargo test --locked -p cmtraceopen-parser— passed;wasm32-unknown-unknownparser check — passed;git diff --check, exact base/head, and clean-worktree checks — passed;Review and merge gate
This is intentionally a draft. The prior hosted review found two valid issues; focused RED
2804a5d4and GREENde0957d4correct them, and both threads have evidence-backed replies. Fresh exact-head CodeRabbit, GitHub Copilot, all CI (including Windows), unresolved-thread review, and a final exact base/head merge guard are required before it can be considered for merge.Tracks #319. Program board: #317.
Summary by CodeRabbit
New Features
Tests