feat(sccm): seal client evidence admission authority - #457
Conversation
adamgell
added
sccm
📝 WalkthroughWalkthroughThe PR adds bounded CCM scanning and SCCM client evidence admission. Intake artifacts now support byte-length and SHA-256 bindings. Admission validates payloads, encoding, provenance, limits, extraction profiles, ordering, and integrity seals. ChangesSCCM evidence authority
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant IntakeBundle
participant admit_client_evidence
participant scan_logical_records_bounded
participant SccmExtractionProfile
participant IntegritySeal
IntakeBundle->>admit_client_evidence: canonical assessment and captured payloads
admit_client_evidence->>scan_logical_records_bounded: decoded CCM content and record limit
scan_logical_records_bounded-->>admit_client_evidence: bounded records and scan status
admit_client_evidence->>SccmExtractionProfile: artifact family
SccmExtractionProfile-->>admit_client_evidence: family-bound profile
admit_client_evidence->>IntegritySeal: ordered evidence projection
IntegritySeal-->>admit_client_evidence: deterministic SHA-256 seal
Possibly related issues
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 |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Pull request overview
This PR extends the SCCM client intake pipeline by introducing an evidence “admission authority” that can fail-closed on mismatched payload length/digest, malformed CCM framing, and bounded-work ceilings, producing an opaque sealed evidence capability for downstream reducers.
Changes:
- Adds optional intake-owned
declaredByteLengthandcontentSha256bindings (paired + validated) to enable exact payload-length/digest verification during admission. - Introduces a bytes-only admission API (
SccmClientCapturedPayload+admit_client_evidence) that reassesses canonical intake, validates payload integrity, frames bounded CCM logical records, and seals deterministic evidence with an integrity hash. - Adds bounded CCM logical-record scanning support and expands synthetic tests/docs to cover admission failure modes and legacy/native projection expectations.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src-tauri/tests/sccm_client_manifest.rs | Updates manifest projection tests to assert completeness is preserved while content binding remains unset for native/legacy projections. |
| src-tauri/src/sccm/manifest.rs | Ensures legacy/native manifest-to-bundle projections explicitly leave new binding fields as None. |
| docs/sccm/preparation/issue-319-client-intake.md | Documents the required single-handle native binding workflow and clarifies legacy fixtures’ assessment-only status. |
| crates/cmtraceopen-parser/tests/sccm_client_intake.rs | Updates synthetic intake fixtures to include the new optional binding fields (defaulting to None). |
| crates/cmtraceopen-parser/tests/sccm_client_admission_authority.rs | Adds a public contract test asserting bytes-only admission succeeds only with intake-bound length/digest authority. |
| crates/cmtraceopen-parser/src/sccm/keys.rs | Adds family-bound profile selection for admitted evidence and adjusts built-in experimental profile recognition. |
| crates/cmtraceopen-parser/src/sccm/client/mod.rs | Wires in the new admission module and exposes the admission API types/functions. |
| crates/cmtraceopen-parser/src/sccm/client/intake.rs | Adds/validates declared_byte_length + content_sha256 in intake artifacts/fragments and exposes helpers for admission. |
| crates/cmtraceopen-parser/src/sccm/client/authority_contract_tests.rs | Adds extensive contract tests for admission fail-closed behavior, readiness locality, and profile binding. |
| crates/cmtraceopen-parser/src/sccm/client/admission.rs | Implements the admission authority: integrity checks, bounded decoding/framing/retention, and deterministic sealing. |
| crates/cmtraceopen-parser/src/sccm/client/admission_tests.rs | Adds focused unit tests for admission limits, encoding/BOM rules, determinism, and seal tamper detection. |
| crates/cmtraceopen-parser/src/parser/ccm.rs | Adds a bounded logical-record scan projection to support admission’s fail-closed framing and record-limit enforcement. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (9)
crates/cmtraceopen-parser/src/sccm/client/admission.rs (3)
117-174: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffEach accessor call re-serializes and re-hashes the complete sealed bundle.
evidence,source_coverage,extract_keys_for_artifact, andrequire_captured_sourceall callverify_integrity.verify_integritycallscompute_integrity_seal, which serializes every retained record and hashes up toMAX_SCCM_CLIENT_ADMISSION_SEAL_BYTES. A reducer that queries coverage per source group therefore pays a full multi-megabyte serialization and SHA-256 pass per query.require_captured_sourcepays it once throughsource_coverage, andextract_keys_for_artifactpays it again before extraction.The fail-closed intent is clear, so do not remove the check. Consider giving reducers one explicit verified view, for example a crate-private
verified(&self) -> Result<VerifiedView<'_>, _>that verifies once and then exposes the accessors without re-verification. Keep the current verifying methods for single-shot callers.🤖 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/client/admission.rs` around lines 117 - 174, Introduce a crate-private verified view for SccmClientAdmittedEvidence that calls verify_integrity once and exposes evidence, source coverage, and artifact key extraction accessors without repeating verification. Update reducer paths, including require_captured_source, to acquire and reuse this verified view across related queries, while preserving the existing verifying methods for single-shot callers and retaining fail-closed integrity checks.
710-722: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTwo hex helpers and two SHA-256 validators now exist in the crate.
digest_hexhere andBoundedIntegrityWriter::finishat lines 632-638 both format a digest as lowercase hex with the same per-byteformat!loop.is_lowercase_sha256also duplicates the digest-shape check thatintake.rsperforms throughis_sha256_digest. Two independent shape checks can drift, and drift here changes which bindings intake accepts versus which bindings admission trusts.Extract one
fn hex_digest(bytes: impl AsRef<[u8]>) -> Stringand share a single digest-shape predicate betweenintake.rsandadmission.rs.🤖 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/client/admission.rs` around lines 710 - 722, Consolidate the duplicate lowercase digest formatting and validation helpers by introducing one shared hex_digest(bytes: impl AsRef<[u8]>) function and one shared SHA-256 digest-shape predicate. Update BoundedIntegrityWriter::finish, the local digest_hex/is_lowercase_sha256 usage in admission.rs, and intake.rs’s is_sha256_digest path to reuse these shared symbols so intake acceptance and admission trust apply identical validation.
366-382: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valuePass
remaining_logical_recordstoscan_logical_records_bounded.This prevents a late payload from retaining up to 4,096 records before admission rejects the bundle. A zero limit sets
record_limit_exceededwhen the scanner encounters a record. The scanner still processes the input after reaching the limit, so this change limits retained records but does not fully bound scan work.🤖 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/client/admission.rs` around lines 366 - 382, Update the call to scan_logical_records_bounded in the admission flow to pass remaining_logical_records as its record-limit argument instead of the fixed MAX_SCCM_CLIENT_ADMISSION_LOGICAL_RECORDS value. Preserve the existing limit, framing, malformed-input, and checked subtraction handling.crates/cmtraceopen-parser/src/sccm/client/admission_tests.rs (2)
592-622: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the redundant
is_errassertion.
admission_erroralready fails the test when the result isOk. Lines 610-613 duplicate that check and consume readability.♻️ Proposed simplification
- let result = admit_client_evidence(&bundle, &assessment, &payloads); - assert!( - result.is_err(), - "two individually bounded rotations totaling 4,098 records were admitted" - ); let error = admission_error( - result, + admit_client_evidence(&bundle, &assessment, &payloads), "the logical-record cap must apply across the complete bundle", );🤖 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/client/admission_tests.rs` around lines 592 - 622, In admission_enforces_logical_record_cap_across_all_payloads, remove the separate result.is_err assertion and pass result directly to admission_error, which already validates that admission fails. Preserve the existing error-message assertion.
571-590: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the retained-evidence budget stays below its cap in this test.
This test asserts
IntegritySealLimitExceeded. The fixture uses one record with 700,000 NUL characters. The retained-evidence check runs before the seal computation inadmit_client_evidence. The test therefore depends on 700,000 retained bytes remaining underMAX_SCCM_CLIENT_ADMISSION_RETAINED_EVIDENCE_BYTESwhile the JSON-escaped form (six bytes per NUL) exceeds the seal cap. Add a comment or a derived assertion that ties the fixture size to both constants. A future change of either cap would otherwise flip this test to a different error without an obvious cause.🤖 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/client/admission_tests.rs` around lines 571 - 590, Update admission_applies_the_integrity_seal_cap_independently_of_retained_memory to explicitly verify the fixture remains within MAX_SCCM_CLIENT_ADMISSION_RETAINED_EVIDENCE_BYTES while its escaped representation exceeds the integrity seal cap. Use derived assertions or a focused comment tied to the existing constants, preserving the expected IntegritySealLimitExceeded result.src-tauri/tests/sccm_client_manifest.rs (1)
298-301: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCompare the error value so a failure reports the observed variant.
assert!(matches!(...))prints only the pattern on failure.SccmClientEvidenceAdmissionErroris compared withassert_eq!in the parser tests, so it implementsPartialEqandDebug. Useassert_eq!here to show which error occurred.♻️ Proposed change
- assert!(matches!( - admit_client_evidence(&bundle, &assessment, &[payload]), - Err(SccmClientEvidenceAdmissionError::MissingContentBinding) - )); + assert_eq!( + admit_client_evidence(&bundle, &assessment, &[payload]).err(), + Some(SccmClientEvidenceAdmissionError::MissingContentBinding) + );This change requires
SccmClientAdmittedEvidenceto implementDebugforResult::err()reporting; if it does not, keepmatches!and add the error text to the message instead.🤖 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_manifest.rs` around lines 298 - 301, Replace the matches! assertion around admit_client_evidence with an assert_eq! comparison against the expected MissingContentBinding error, ensuring SccmClientAdmittedEvidence implements Debug as required for Result error reporting. If that result type cannot implement Debug, retain matches! and add an assertion failure message containing the observed error text.src-tauri/src/sccm/manifest.rs (1)
123-125: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRecord why the content binding stays absent here.
The native projection leaves
declared_byte_lengthandcontent_sha256unset, so admission fails closed withMissingContentBinding. The manifest already carries a copied byte count. A future author could populate these fields from a separate stat or hash step.docs/sccm/preparation/issue-319-client-intake.mdLines 139-146 forbids that, because a file replacement between the steps would bind the manifest to bytes that were never supplied. Add a one-line comment at this site that points to the single-handle requirement.🤖 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/manifest.rs` around lines 123 - 125, Add a one-line comment immediately above the unset declared_byte_length and content_sha256 fields in the native projection, documenting that they must remain absent because binding requires the single file handle used for intake and must not rely on separate stat or hash steps.crates/cmtraceopen-parser/src/sccm/client/authority_contract_tests.rs (2)
61-69: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify the
relative_pathconstruction.The chained
then/or_elseexpresses one three-way choice. A singleifreads clearer and removes the duplicatedphysicaltest.♻️ Proposed simplification
- let relative_path = (physical && group != "unknown") - .then(|| format!("evidence/{group}/current/{basename}")) - .or_else(|| { - (physical && group == "unknown").then(|| format!("evidence/{group}/{basename}")) - }); + let relative_path = physical.then(|| { + if group == "unknown" { + format!("evidence/{group}/{basename}") + } else { + format!("evidence/{group}/current/{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 `@crates/cmtraceopen-parser/src/sccm/client/authority_contract_tests.rs` around lines 61 - 69, Update the relative_path construction to use one conditional expression: return the “current” path when physical is true and group is not "unknown", the fallback group path when physical is true with an unknown group, and None otherwise. Replace the chained then/or_else logic without changing the resulting paths.
17-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShare the fixture helpers instead of copying them.
digestand the CCM record builder exist in three places: here, inadmission_tests.rs(Lines 10-15 and 83-93), and incrates/cmtraceopen-parser/tests/sccm_client_admission_authority.rs(Lines 8-23). The record template must stay identical for the binding assertions to keep their meaning. Extract one shared test-support module and use it in all three files.🤖 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/client/authority_contract_tests.rs` around lines 17 - 34, Extract the duplicated digest and CCM record-builder helpers into one shared test-support module, then update authority_contract_tests.rs, admission_tests.rs, and sccm_client_admission_authority.rs to import and reuse those helpers. Preserve the existing helper behavior and identical record template, including formatting and fixture values, so all binding assertions retain their meaning.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/cmtraceopen-parser/src/parser/ccm.rs`:
- Around line 374-394: The caller of scan_logical_records_bounded, specifically
admit_client_evidence, must pass the current remaining_logical_records budget
for each payload instead of the fixed MAX_SCCM_CLIENT_ADMISSION_LOGICAL_RECORDS
value. Update the budget before each scan as records are admitted so scanning
stops at the bundle-wide remaining limit and does not materialize records beyond
it.
---
Nitpick comments:
In `@crates/cmtraceopen-parser/src/sccm/client/admission_tests.rs`:
- Around line 592-622: In
admission_enforces_logical_record_cap_across_all_payloads, remove the separate
result.is_err assertion and pass result directly to admission_error, which
already validates that admission fails. Preserve the existing error-message
assertion.
- Around line 571-590: Update
admission_applies_the_integrity_seal_cap_independently_of_retained_memory to
explicitly verify the fixture remains within
MAX_SCCM_CLIENT_ADMISSION_RETAINED_EVIDENCE_BYTES while its escaped
representation exceeds the integrity seal cap. Use derived assertions or a
focused comment tied to the existing constants, preserving the expected
IntegritySealLimitExceeded result.
In `@crates/cmtraceopen-parser/src/sccm/client/admission.rs`:
- Around line 117-174: Introduce a crate-private verified view for
SccmClientAdmittedEvidence that calls verify_integrity once and exposes
evidence, source coverage, and artifact key extraction accessors without
repeating verification. Update reducer paths, including require_captured_source,
to acquire and reuse this verified view across related queries, while preserving
the existing verifying methods for single-shot callers and retaining fail-closed
integrity checks.
- Around line 710-722: Consolidate the duplicate lowercase digest formatting and
validation helpers by introducing one shared hex_digest(bytes: impl AsRef<[u8]>)
function and one shared SHA-256 digest-shape predicate. Update
BoundedIntegrityWriter::finish, the local digest_hex/is_lowercase_sha256 usage
in admission.rs, and intake.rs’s is_sha256_digest path to reuse these shared
symbols so intake acceptance and admission trust apply identical validation.
- Around line 366-382: Update the call to scan_logical_records_bounded in the
admission flow to pass remaining_logical_records as its record-limit argument
instead of the fixed MAX_SCCM_CLIENT_ADMISSION_LOGICAL_RECORDS value. Preserve
the existing limit, framing, malformed-input, and checked subtraction handling.
In `@crates/cmtraceopen-parser/src/sccm/client/authority_contract_tests.rs`:
- Around line 61-69: Update the relative_path construction to use one
conditional expression: return the “current” path when physical is true and
group is not "unknown", the fallback group path when physical is true with an
unknown group, and None otherwise. Replace the chained then/or_else logic
without changing the resulting paths.
- Around line 17-34: Extract the duplicated digest and CCM record-builder
helpers into one shared test-support module, then update
authority_contract_tests.rs, admission_tests.rs, and
sccm_client_admission_authority.rs to import and reuse those helpers. Preserve
the existing helper behavior and identical record template, including formatting
and fixture values, so all binding assertions retain their meaning.
In `@src-tauri/src/sccm/manifest.rs`:
- Around line 123-125: Add a one-line comment immediately above the unset
declared_byte_length and content_sha256 fields in the native projection,
documenting that they must remain absent because binding requires the single
file handle used for intake and must not rely on separate stat or hash steps.
In `@src-tauri/tests/sccm_client_manifest.rs`:
- Around line 298-301: Replace the matches! assertion around
admit_client_evidence with an assert_eq! comparison against the expected
MissingContentBinding error, ensuring SccmClientAdmittedEvidence implements
Debug as required for Result error reporting. If that result type cannot
implement Debug, retain matches! and add an assertion failure message containing
the observed error text.
🪄 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: 3db723dc-f337-4e6a-8d45-a8bf995421e7
📒 Files selected for processing (12)
crates/cmtraceopen-parser/src/parser/ccm.rscrates/cmtraceopen-parser/src/sccm/client/admission.rscrates/cmtraceopen-parser/src/sccm/client/admission_tests.rscrates/cmtraceopen-parser/src/sccm/client/authority_contract_tests.rscrates/cmtraceopen-parser/src/sccm/client/intake.rscrates/cmtraceopen-parser/src/sccm/client/mod.rscrates/cmtraceopen-parser/src/sccm/keys.rscrates/cmtraceopen-parser/tests/sccm_client_admission_authority.rscrates/cmtraceopen-parser/tests/sccm_client_intake.rsdocs/sccm/preparation/issue-319-client-intake.mdsrc-tauri/src/sccm/manifest.rssrc-tauri/tests/sccm_client_manifest.rs
|
Published exact reviewed correction f6c458a. Two hosted blockers are now fixed: public family-bearing profile forgery returns no keys plus UnvalidatedProfile, and bounded CCM scanning consumes the true remaining bundle budget (4096 then 1 in the regression) without materializing more than 4096 records. Panic-safe observer cleanup is also pinned. Independent exact-range review: GO/no findings. Focused authority/aggregate/cleanup tests, full parser (443 units plus integrations), strict parser Clippy, wasm32, TypeScript, scoped Rustfmt/diff, and local CodeRabbit zero findings pass. Both handled review threads are resolved; fresh exact-head hosted CodeRabbit, Copilot, and CI remain merge gates. |
|
@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 17 minutes. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.
Suppressed comments (1)
crates/cmtraceopen-parser/src/sccm/client/admission.rs:44
- The doc comment for
MAX_SCCM_CLIENT_ADMISSION_LOGICAL_RECORDSsays it limits “logical CCM records”, but the underlying bounded CCM scan enforces the limit against all logical records materialized during scanning (including unmatched plain-text/error records), not justLogFormat::Ccmrecords. This mismatch can mislead future maintainers and consumers of the error (LogicalRecordLimitExceeded).
/// Maximum logical CCM records retained from one admitted client bundle.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
crates/cmtraceopen-parser/src/parser/ccm.rs (1)
402-421: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRestore the outer observation state if
observe_bounded_scansis nested.
observations.replace(Some(Vec::new()))overwrites the installed state before the assertion fails. The outer call then loses its collected observations, and the outerexpectcan report a misleading state. Check first, then install.♻️ Proposed change
BOUNDED_SCAN_OBSERVATIONS.with(|observations| { assert!( - observations.replace(Some(Vec::new())).is_none(), + observations.borrow().is_none(), "bounded scan observation cannot be nested" ); + observations.replace(Some(Vec::new())); });🤖 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/parser/ccm.rs` around lines 402 - 421, Update observe_bounded_scans to inspect the existing BOUNDED_SCAN_OBSERVATIONS state before installing a new Vec, so a nested call fails without overwriting the outer observation buffer. Preserve the outer state for cleanup and collection after the assertion.src-tauri/src/sccm/manifest.rs (1)
124-125: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRecord why the native projection drops the manifest content binding.
The native manifest already carries
contentSha256andbytesCopied, as the fixture insrc-tauri/tests/sccm_client_manifest.rsshows. This projection deliberately discards both, so no native bundle can produce admissible evidence yet. Without an inline rationale, a later change may copy the manifest values here and defeat the single-handle authority rule documented indocs/sccm/preparation/issue-319-client-intake.md. Add a short comment that points to the pending native handle work. The legacy projection at Lines 872-873 has no manifest values to copy, so it needs no comment.♻️ Proposed comment
fragment_complete: Some(source.fragment_complete), + // The manifest digest and byte count are not single-handle + // capture authority, so they must not become an admission + // binding. The pending native capture work populates these + // fields from one open source handle instead. declared_byte_length: None, content_sha256: None,🤖 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/manifest.rs` around lines 124 - 125, Add a short inline comment beside declared_byte_length and content_sha256 in the native manifest projection explaining that these fields are intentionally omitted pending native handle work, and reference the single-handle authority rule or its documented issue. Do not modify the legacy projection.crates/cmtraceopen-parser/src/sccm/client/admission.rs (1)
275-437: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider splitting
admit_client_evidenceinto named phases.The function performs eight distinct steps: bound checks, canonical reassessment, eligibility selection, group readiness, payload count reconciliation, per-payload decode and scan, evidence normalization, and sealing. Extracting
select_eligible_fragments,compute_admitted_source_groups, andnormalize_scanned_recordswould keep each fail-closed rule independently testable without changing behavior.🤖 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/client/admission.rs` around lines 275 - 437, Refactor admit_client_evidence into named phases without changing behavior: extract eligibility selection into select_eligible_fragments, source-group readiness into compute_admitted_source_groups, and per-record normalization and identity/retained-size validation into normalize_scanned_records. Keep the existing bound checks, canonical assessment, payload reconciliation, decoding/scanning, and integrity sealing flow, preserving all fail-closed errors and validation limits.crates/cmtraceopen-parser/src/sccm/client/admission_tests.rs (1)
418-469: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin the exact rejection reason in the admission tests that assert only
is_err(). Admission distinguishes 24 error variants, and each one encodes a separate fail-closed rule. These three tests assert only that admission failed, so a change that swaps one rejection reason for another still passes. The neighbouring tests already compare againstSccmClientEvidenceAdmissionErrorvariants through theadmission_errorhelper.
crates/cmtraceopen-parser/src/sccm/client/admission_tests.rs#L418-L469: assert the variant for the capped, incomplete, unknown-profile, malformed-CCM, and invalid-offset cases.crates/cmtraceopen-parser/src/sccm/client/admission_tests.rs#L219-L253: assertMissingPayload,ExtraPayload,DuplicatePayload, andPayloadIntegrityMismatchfor the four cases in order.crates/cmtraceopen-parser/src/sccm/client/authority_contract_tests.rs#L439-L465: assert the variant that proves a non-CCM source never becomes eligible.🤖 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/client/admission_tests.rs` around lines 418 - 469, Strengthen the admission assertions by replacing bare is_err() checks with admission_error comparisons against the precise SccmClientEvidenceAdmissionError variant: in crates/cmtraceopen-parser/src/sccm/client/admission_tests.rs:418-469 cover capped, incomplete, unknown-profile, malformed-CCM, and invalid-offset cases; in crates/cmtraceopen-parser/src/sccm/client/admission_tests.rs:219-253 assert MissingPayload, ExtraPayload, DuplicatePayload, and PayloadIntegrityMismatch in order; and in crates/cmtraceopen-parser/src/sccm/client/authority_contract_tests.rs:439-465 assert the variant proving non-CCM sources are ineligible. Reuse the neighboring admission_error helper and existing error variants.crates/cmtraceopen-parser/src/sccm/client/intake.rs (1)
1530-1552: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueShare the SHA-256 validator
is_sha256_digestrejects uppercase hexadecimal values. Share this validator with admission to keep both paths aligned with the lowercase contract.🤖 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/client/intake.rs` around lines 1530 - 1552, The SHA-256 validation in validate_content_binding must use the shared admission validator so uppercase handling and the lowercase contract remain consistent. Replace the local is_sha256_digest dependency with the existing shared validator used by admission, preserving the current invalid-content-binding error behavior.crates/cmtraceopen-parser/src/sccm/client/authority_contract_tests.rs (1)
680-686: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBind the fixture to
EXPERIMENTAL_VERSION_PREFIX.Make the private constant crate-visible, use it for
configmgr_version_prefixes, and deriveselected_configmgr_versionfrom the same prefix. This keeps the test focused on thevalidated_artifact_familiesrule.🤖 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/client/authority_contract_tests.rs` around lines 680 - 686, Make the existing experimental version-prefix constant crate-visible, then update the SccmExtractionProfile fixture in the caller-constructed test to use EXPERIMENTAL_VERSION_PREFIX for configmgr_version_prefixes and derive selected_configmgr_version from that same prefix. Leave validated_artifact_families unchanged so the test remains focused on that rule.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/cmtraceopen-parser/src/sccm/client/admission.rs`:
- Around line 530-540: Update assess_client_intake’s fragment filtering so
fragments with encoding: None or an unsupported encoding are excluded from both
eligible and admitted_source_groups before payload decoding. Preserve admission
of supported encodings and the existing local-gap behavior, and add a contract
test covering a missing-encoding fragment without aborting unrelated source
admission.
---
Nitpick comments:
In `@crates/cmtraceopen-parser/src/parser/ccm.rs`:
- Around line 402-421: Update observe_bounded_scans to inspect the existing
BOUNDED_SCAN_OBSERVATIONS state before installing a new Vec, so a nested call
fails without overwriting the outer observation buffer. Preserve the outer state
for cleanup and collection after the assertion.
In `@crates/cmtraceopen-parser/src/sccm/client/admission_tests.rs`:
- Around line 418-469: Strengthen the admission assertions by replacing bare
is_err() checks with admission_error comparisons against the precise
SccmClientEvidenceAdmissionError variant: in
crates/cmtraceopen-parser/src/sccm/client/admission_tests.rs:418-469 cover
capped, incomplete, unknown-profile, malformed-CCM, and invalid-offset cases; in
crates/cmtraceopen-parser/src/sccm/client/admission_tests.rs:219-253 assert
MissingPayload, ExtraPayload, DuplicatePayload, and PayloadIntegrityMismatch in
order; and in
crates/cmtraceopen-parser/src/sccm/client/authority_contract_tests.rs:439-465
assert the variant proving non-CCM sources are ineligible. Reuse the neighboring
admission_error helper and existing error variants.
In `@crates/cmtraceopen-parser/src/sccm/client/admission.rs`:
- Around line 275-437: Refactor admit_client_evidence into named phases without
changing behavior: extract eligibility selection into select_eligible_fragments,
source-group readiness into compute_admitted_source_groups, and per-record
normalization and identity/retained-size validation into
normalize_scanned_records. Keep the existing bound checks, canonical assessment,
payload reconciliation, decoding/scanning, and integrity sealing flow,
preserving all fail-closed errors and validation limits.
In `@crates/cmtraceopen-parser/src/sccm/client/authority_contract_tests.rs`:
- Around line 680-686: Make the existing experimental version-prefix constant
crate-visible, then update the SccmExtractionProfile fixture in the
caller-constructed test to use EXPERIMENTAL_VERSION_PREFIX for
configmgr_version_prefixes and derive selected_configmgr_version from that same
prefix. Leave validated_artifact_families unchanged so the test remains focused
on that rule.
In `@crates/cmtraceopen-parser/src/sccm/client/intake.rs`:
- Around line 1530-1552: The SHA-256 validation in validate_content_binding must
use the shared admission validator so uppercase handling and the lowercase
contract remain consistent. Replace the local is_sha256_digest dependency with
the existing shared validator used by admission, preserving the current
invalid-content-binding error behavior.
In `@src-tauri/src/sccm/manifest.rs`:
- Around line 124-125: Add a short inline comment beside declared_byte_length
and content_sha256 in the native manifest projection explaining that these
fields are intentionally omitted pending native handle work, and reference the
single-handle authority rule or its documented issue. Do not modify the legacy
projection.
🪄 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: a8e9ac59-a76d-4294-a00c-9e17f313da22
📒 Files selected for processing (12)
crates/cmtraceopen-parser/src/parser/ccm.rscrates/cmtraceopen-parser/src/sccm/client/admission.rscrates/cmtraceopen-parser/src/sccm/client/admission_tests.rscrates/cmtraceopen-parser/src/sccm/client/authority_contract_tests.rscrates/cmtraceopen-parser/src/sccm/client/intake.rscrates/cmtraceopen-parser/src/sccm/client/mod.rscrates/cmtraceopen-parser/src/sccm/keys.rscrates/cmtraceopen-parser/tests/sccm_client_admission_authority.rscrates/cmtraceopen-parser/tests/sccm_client_intake.rsdocs/sccm/preparation/issue-319-client-intake.mdsrc-tauri/src/sccm/manifest.rssrc-tauri/tests/sccm_client_manifest.rs
|
@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 9 minutes. |
Add the reviewed pure-Rust client evidence admission boundary with intake-owned content binding, bounded shared CCM framing, opaque sealed authority, family-bound extraction profiles, and source-local coverage gaps. Native same-handle capture binding and live Windows validation remain future gated work.
Part of #319.
Summary
declaredByteLengthand lowercasecontentSha256bindings to complete captured client artifacts, with paired/fail-closed validation.None; even exact native-manifest-validated bytes remainMissingContentBinding.Architecture boundaries
cmtraceopen-parserremains pure Rust and wasm32-compatible.ParserKind::Sccmand no duplicate parser.LogEntry, generic collection-manifest, andArtifactStatuscontracts are unchanged.Review evidence
09aafd4a70fb70f7db90a4ebb09c10bb1ee12020; current live integration after the disjoint private-destination merge:03ea96c9f17e000baeab19fa205d7184a68ac2ec.05f5d5ed2138042871f0bebc3b9f081ffb660e99f6c458ad..05f5d5ed): GO, no P0-P3 finding; the parent bug was reproduced from control flow and the corrected source-local gap, sealed readiness set, public grammar, full parser, Clippy, and wasm boundaries were verified.ExtraPayloadand would invalidate the local-gap success test.Verification
cargo test --locked -p cmtraceopen-parser sccm::client::admission_tests— 15 passedcargo test --locked -p cmtraceopen-parser sccm::client::authority_contract_tests— 17 passedcargo test --locked -p cmtraceopen-parser --test sccm_client_admission_authority— 1 passedcargo test --locked -p cmtraceopen-parser --test sccm_client_intake— 65 passedcargo test --locked -p cmtrace-open --test sccm_client_manifest --no-default-features --features sccm-diagnostics— 21 passedcargo test --locked -p cmtraceopen-parser— passedcargo check --locked -p cmtrace-open --no-default-features --features sccm-diagnostics— passed-D warnings— passedwasm32-unknown-unknowncheck — passednpx --offline tsc --noEmit— passedgit diff --check, and clean worktree — passedRepository-wide formatting still reports inherited drift in unrelated files byte-identical between base and head; this range adds none.
This draft remains blocked on exact-head hosted CI, CodeRabbit, Copilot, zero unresolved threads, and a repeated live base/head/tree guard. It does not complete #319; same-handle native capture binding and live Windows validation remain open.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation