feat(sccm): add native client manifest reader - #440
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds a feature-gated SCCM diagnostic module with manifest contracts, secure cross-platform bundle traversal, native and legacy manifest loading, evidence verification, client-intake projection, and integration tests. ChangesSCCM manifest intake
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant ManifestLoader
participant VerifiedBundleRoot
participant EvidenceFile
participant ClientIntake
Caller->>ManifestLoader: read SCCM bundle
ManifestLoader->>VerifiedBundleRoot: verify private bundle root
ManifestLoader->>VerifiedBundleRoot: open evidence path
VerifiedBundleRoot->>EvidenceFile: read and hash verified file
EvidenceFile-->>ManifestLoader: return validated evidence
ManifestLoader->>ClientIntake: project artifacts and capture gaps
ClientIntake-->>Caller: return intake bundle
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Comment |
github-actions
Bot
added
enhancement
|
@coderabbitai review Please review exact head |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
base='26a9a5ee6397696475b3ea709d044f16aa8104fd'
head='84079b8c30bd1ab52d3c7711ea50ee58a92276b2'
printf '%s\n' '=== checked-out commit ==='
git rev-parse HEAD
printf '%s\n' '=== exact delta files ==='
git diff --name-status "$base" "$head"
printf '%s\n' '=== diff summary ==='
git diff --stat "$base" "$head"
printf '%s\n' '=== Rust outline for changed files ==='
while IFS= read -r path; do
[ -n "$path" ] || continue
case "$path" in
*.rs)
printf '\n--- %s ---\n' "$path"
ast-grep outline "$path" --items all || true
;;
esac
done < <(git diff --name-only "$base" "$head")Length of output: 1841 🏁 Script executed: #!/bin/bash
set -euo pipefail
base='26a9a5ee6397696475b3ea709d044f16aa8104fd'
head='84079b8c30bd1ab52d3c7711ea50ee58a92276b2'
for path in \
src-tauri/src/sccm/contract.rs \
src-tauri/src/sccm/manifest.rs \
src-tauri/src/sccm/mod.rs \
src-tauri/src/sccm/private_fs.rs \
src-tauri/tests/sccm_client_manifest.rs
do
printf '\n=== outline: %s ===\n' "$path"
ast-grep outline "$path" --items all || true
done
printf '\n=== focused symbol and error searches ===\n'
rg -n -i -C 4 \
'NtCreateFile|ntstatus|status_to|reparse|hard.?link|no_follow|nofollow|openat|relative|manifest\.json|legacy|digest|sha256|blake3|max_|limit|bounded|capture.?gap|privacy|path.*error|error.*path|display' \
src-tauri/src/sccm/contract.rs \
src-tauri/src/sccm/manifest.rs \
src-tauri/src/sccm/private_fs.rs \
src-tauri/tests/sccm_client_manifest.rs || true
printf '\n=== exact diff: private filesystem layer ===\n'
git diff --unified=40 "$base" "$head" -- src-tauri/src/sccm/private_fs.rs
printf '\n=== exact diff: manifest layer ===\n'
git diff --unified=25 "$base" "$head" -- src-tauri/src/sccm/manifest.rsLength of output: 234109 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '=== all physical byte-limit definitions and uses ==='
rg -n -i -C 3 \
'MAX_.*(BYTE|SIZE|EVIDENCE|ARTIFACT)|max_bytes_per_source|maxBytesPerSource|bytes_copied|bytesCopied|read_bounded_file|sha256_exact_file' \
src-tauri/src/sccm \
src-tauri/tests/sccm_client_manifest.rs \
src-tauri/Cargo.toml \
src-tauri/src/lib.rs || true
printf '%s\n' '=== parser SCCM contract outline and limits ==='
fd -a -t f -e rs . | while IFS= read -r path; do
case "$path" in
*parser*/*sccm*.rs|*parser*/sccm.rs)
printf '\n--- %s ---\n' "$path"
ast-grep outline "$path" --items all || true
rg -n -i -C 3 'MAX_.*(BYTE|SIZE|EVIDENCE|ARTIFACT)|byte|size|limit' "$path" || true
;;
esac
done
printf '%s\n' '=== narrow review ranges ==='
nl -ba src-tauri/src/sccm/manifest.rs | sed -n '119,176p;576,666p'
nl -ba src-tauri/tests/sccm_client_manifest.rs | sed -n '505,559p'Length of output: 127027 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '=== parser source files that define SCCM resource limits ==='
rg -n -i -C 3 \
'MAX_.*(BYTE|SIZE|ARTIFACT|MANIFEST|PAYLOAD)|268_435_456|1_073_741_824|ManifestLimitExceeded|aggregate.*byte|per.artifact' \
crates/cmtraceopen-parser/src || true
printf '%s\n' '=== client intake source declarations and limit checks ==='
rg -n -i -C 5 \
'SccmClientIntake|assess_client_intake|MAX_SCCM_CLIENT|bytes_copied|byte.*limit|limit.*byte' \
crates/cmtraceopen-parser/src || true
printf '%s\n' '=== native reader limit and evidence-read implementation ==='
sed -n '137,176p;576,666p' src-tauri/src/sccm/manifest.rs
printf '%s\n' '=== native reader tests for source limits ==='
sed -n '505,559p' src-tauri/tests/sccm_client_manifest.rsLength of output: 156831
I found no additional defects in the reviewed Windows handle-relative traversal, NTSTATUS-to-Win32 fallback mapping, manifest entry limits, or path-disclosure surfaces. I could not execute tests in this sandbox. ✅ Action performedReview finished.
|
There was a problem hiding this comment.
Pull request overview
Adds a reader-only native SCCM client “intake bundle” manifest boundary to the Tauri backend (src-tauri), validating a native manifest contract and projecting it into the existing pure Rust SCCM intake model in cmtraceopen-parser, with strong privacy and filesystem-safety constraints.
Changes:
- Introduces SCCM native manifest contract types + canonical ID/provenance helpers and bounded decoding.
- Implements native/legacy manifest reading, validation (ordering, dedup, limits, digest binding), and projection into
SccmClientIntakeBundle. - Adds handle-bound, no-follow filesystem traversal + extensive tests covering gaps, privacy boundaries, and unsafe filesystem constructs.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src-tauri/tests/sccm_client_manifest.rs | End-to-end tests for native + legacy manifest reading, privacy, ordering/dedup, caps, and unsafe filesystem cases. |
| src-tauri/src/sccm/private_fs.rs | Handle-bound, no-follow directory traversal and private-root verification across Unix/Windows. |
| src-tauri/src/sccm/mod.rs | SCCM module wiring and exports for the native-reader boundary. |
| src-tauri/src/sccm/manifest.rs | Native manifest/legacy fallback reader, validation pipeline, and pure-model projection. |
| src-tauri/src/sccm/contract.rs | Native manifest v1 wire contract + bounded shared entry decoding and canonical identity helpers. |
| src-tauri/src/lib.rs | Exposes sccm module behind the sccm-diagnostics feature. |
| src-tauri/Cargo.toml | Adds sccm-diagnostics feature, enables required windows crate features, and registers the SCCM manifest test. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (14)
src-tauri/src/sccm/contract.rs (1)
394-399: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueBuild the hex string without per-byte allocations.
sha256_bytescreates a temporaryStringfor each digest byte. Write into one preallocatedString, or use an existing hex dependency.sha2 = "0.11"resolves to0.11.0, andSha256::digestremains supported.🤖 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/contract.rs` around lines 394 - 399, Update sha256_bytes to construct the digest’s hexadecimal representation in one preallocated String rather than mapping each byte through format!, or reuse an existing hex-encoding dependency. Preserve the current lowercase two-character-per-byte output and continue using Sha256::digest.src-tauri/src/sccm/private_fs.rs (4)
120-120: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
#[cfg(target_os = "windows")]for the Windows-gated items.The coding guidelines require Windows-specific code to be gated with
#[cfg(target_os = "windows")]. This file uses#[cfg(windows)]throughout, for example at Lines 120, 130, 140, 419, 431, 593, 613 and 632. The two forms behave the same for supported targets, so this is a convention alignment.As per coding guidelines: "Gate Windows-specific code with
#[cfg(target_os = "windows")]and use thewindows/winregcrates for Windows functionality."🤖 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/private_fs.rs` at line 120, Replace every Windows-specific #[cfg(windows)] attribute in private_fs.rs with #[cfg(target_os = "windows")], including the gated items around the identified locations, while leaving the surrounding implementations unchanged.Source: Coding guidelines
668-675: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse the
windowscrate constants instead of redefining them.Line 671 defines a local
FILE_ATTRIBUTE_REPARSE_POINT. Line 423 defines a localFILE_FLAG_OPEN_REPARSE_POINT. The file already imports both from thewindowscrate at Line 47 and Lines 615-617. Import the crate constants in these two places to avoid two sources of truth.♻️ Proposed change for `is_reparse_point`
#[cfg(windows)] { use std::os::windows::fs::MetadataExt; - const FILE_ATTRIBUTE_REPARSE_POINT: u32 = 0x0000_0400; - if metadata.file_attributes() & FILE_ATTRIBUTE_REPARSE_POINT != 0 { + use windows::Win32::Storage::FileSystem::FILE_ATTRIBUTE_REPARSE_POINT; + if metadata.file_attributes() & FILE_ATTRIBUTE_REPARSE_POINT.0 != 0 { return true; } }🤖 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/private_fs.rs` around lines 668 - 675, Update the reparse-point checks in is_reparse_point and the code around FILE_FLAG_OPEN_REPARSE_POINT to reuse the existing windows crate constants, importing them where needed and removing the local constant definitions. Preserve the current flag checks and behavior.
387-403: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winClear
O_NONBLOCKon the returned file, asopen_file_no_followdoes.
O_NONBLOCKprevents a blocking open on a FIFO or a device, which is correct. The flag then stays set on the returned descriptor. The test helper at Lines 342-348 clears it, and the Unix test at Line 745 asserts it is clear. The production path does not clear it.Regular-file reads normally ignore
O_NONBLOCK. On some filesystems a short orEAGAINread is still possible, andsha256_exact_fileinsrc-tauri/src/sccm/manifest.rstreats a zero-length read as evidence truncation. Clearing the flag removes that difference between the two open paths.♻️ Extract the flag reset and reuse it on the final component
let opened = unsafe { File::from_raw_fd(descriptor) }; if final_component { - return require_regular_file(opened); + let opened = require_regular_file(opened)?; + clear_nonblock(&opened)?; + return Ok(opened); }Add the helper next to the traversal function:
#[cfg(unix)] fn clear_nonblock(file: &File) -> io::Result<()> { use std::os::fd::AsRawFd; let descriptor = file.as_raw_fd(); // SAFETY: `descriptor` is borrowed from the live `File`; both fcntl calls // operate only on its status flags and preserve every flag except NONBLOCK. let flags = unsafe { libc::fcntl(descriptor, libc::F_GETFL) }; if flags < 0 { return Err(io::Error::last_os_error()); } if unsafe { libc::fcntl(descriptor, libc::F_SETFL, flags & !libc::O_NONBLOCK) } < 0 { return Err(io::Error::last_os_error()); } Ok(()) }
open_file_no_followcan then callclear_nonblockinstead of repeating the fcntl pair.🤖 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/private_fs.rs` around lines 387 - 403, Clear O_NONBLOCK on the final file descriptor opened by the traversal function before returning it, matching the behavior of open_file_no_follow. Extract the existing fcntl-based reset into a clear_nonblock helper and reuse it from both paths, propagating any fcntl errors while preserving all other descriptor flags.
329-330: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove or test-gate the unused platform variants.
open_file_no_followhas no non-test callers. Its only caller is in the Unix-only test module. The Windows and fallback definitions are unused in release builds; the Unix build does not currently fail.🤖 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/private_fs.rs` around lines 329 - 330, Update open_file_no_follow and its platform-specific definitions so they are compiled only for the Unix test configuration where the function is used, or remove the unused Windows and fallback variants. Preserve the Unix test caller and avoid retaining unused release-build definitions.src-tauri/tests/sccm_client_manifest.rs (6)
343-353: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe duplicate-path branch is not covered by this test.
The test is named
reader_rejects_duplicate_artifact_ids_and_relative_paths, but it only asserts"duplicate artifact IDs". The two fixtures are identical clones, sovalidate_native_manifest_structurerejects them at the artifact-ID check on line 219 ofsrc-tauri/src/sccm/manifest.rs. The relative-path check at lines 224-230 never runs.The physical artifact ID is derived from the fingerprint, rotation, and basename, and the relative path is derived from the same values. Two artifacts that share a relative path therefore also share an artifact ID. Confirm whether the relative-path branch is reachable at all. If it is unreachable for physical artifacts, either add a fixture that reaches it or rename the test to match what it verifies.
💚 Proposed rename
#[test] -fn reader_rejects_duplicate_artifact_ids_and_relative_paths() { +fn reader_rejects_duplicate_artifact_ids() {🤖 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 343 - 353, Rename reader_rejects_duplicate_artifact_ids_and_relative_paths to reflect that the fixture only verifies duplicate artifact ID rejection, since identical physical_artifact values fail before relative-path validation. Keep the existing assertions and test behavior unchanged.
113-121: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for a complete captured fragment.
Every physical fixture sets
"state": "captured"with"fragmentComplete": false. No test builds a captured artifact with"fragmentComplete": true. That path reaches theelsebranch atsrc-tauri/src/sccm/manifest.rsline 371 and is accepted, but no test pins that behavior.Add one test that sets
"fragmentComplete": trueon a captured artifact and asserts that the projection reportsfragment_complete: Some(true). This protects thefragment_completeprojection atsrc-tauri/src/sccm/manifest.rsline 84 against a regression that hardcodes the value.🤖 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 113 - 121, Add a test in the SCCM client manifest test coverage that constructs a captured artifact with "fragmentComplete": true, then assert the projected fragment_complete value is Some(true). Reuse the existing captured-artifact fixture or test setup and target the manifest projection path exercised by the existing tests.
560-574: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a Windows hard-link test.
reader_rejects_hard_linked_physical_evidenceis gated with#[cfg(unix)]. The PR objectives claim hard-link rejection on Unix and Windows. Windows supports hard links throughCreateHardLinkW, and the link count is available fromFILE_STANDARD_INFO. Without a#[cfg(windows)]test, the Windows branch of that rejection is unverified.Add an equivalent test gated with
#[cfg(windows)]. Note that creating a hard link on Windows CI may require no special privilege for files on the same volume, sostd::fs::hard_linkshould work in the temp directory.🤖 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 560 - 574, Add a Windows-gated counterpart to reader_rejects_hard_linked_physical_evidence that creates a duplicate file name with std::fs::hard_link in the same temporary bundle, invokes read_sccm_manifest_or_legacy, and asserts the same safe-opening rejection. Keep the existing Unix test unchanged and use #[cfg(windows)] for the new test.
402-435: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStrengthen the symlink assertions.
Both symlink tests assert only that the error message omits a path. Any failure satisfies that assertion. If
verify_bundle_rootoropen_relative_filefailed for an unrelated reason, for example a missing manifest, the tests would still pass and the no-follow guarantee would go unverified.
reader_rejects_a_nonprivate_bundle_directoryat line 439 shows the stronger pattern. It asserts"not private"in addition to the path check. Apply the same pattern here.For
reader_opens_manifest_without_following_a_symlink, also assert that the outside file content is not parsed. The outside file holds{}, which is valid JSON. If the reader followed the symlink, it would fail with a schema error rather than an open error. Asserting the expected error text distinguishes the two outcomes.💚 Proposed assertion
let error = read_sccm_manifest_or_legacy(&bundle_root) .expect_err("reader must not follow the manifest symlink"); + assert!( + error.to_string().contains("cannot be opened safely"), + "expected a no-follow open rejection, got: {error}" + ); assert!(!error .to_string() .contains(&bundle_root.display().to_string()));🤖 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 402 - 435, Strengthen both symlink tests by asserting the specific rejection errors, not only the absence of paths. In reader_rejects_a_symlinked_bundle_root_without_following_it, require the error text to include “not private” alongside the existing path assertion; in reader_opens_manifest_without_following_a_symlink, assert the expected no-follow/open error text so valid JSON from the outside file cannot be mistaken for a schema failure.
88-90: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the second source of truth for evidence content.
physical_artifactreceives acontentargument and derivesbytesCopiedandcontentSha256from it.write_native_bundleignores that argument. It re-derives the bytes from the basename in the match at lines 190-195.All current callers pass matching bytes, so the tests pass today. If a future test passes different bytes for a known basename, the manifest digest and the written file diverge. The failure then surfaces as "SCCM evidence violates content digest coherence" from
src-tauri/src/sccm/manifest.rsline 601, which points at production code rather than at the fixture.Thread the content through instead of re-deriving it.
♻️ Proposed refactor
-fn physical_artifact(rotation: SccmRotation, content: &[u8]) -> Value { +fn physical_artifact(rotation: SccmRotation, content: &[u8]) -> (Value, Vec<u8>) {A smaller change keeps the signature and makes the divergence impossible instead:
fn write_native_bundle(root: &Path, artifacts: &[Value], capture_gaps: &[Value]) { make_private_directory(root); for artifact in artifacts { let relative_path = artifact["relativePath"] .as_str() .expect("physical fixture path"); - let basename = artifact["basename"] - .as_str() - .expect("physical fixture basename"); - let content = match basename { - POLICY_BASENAME => b"policy-current".as_slice(), - "PolicyAgent.log.1" => b"policy-rotation-one".as_slice(), - "PolicyAgent.log.2" => b"policy-rotation-two".as_slice(), - _ => panic!("unexpected physical fixture basename"), - }; + let expected_digest = artifact["contentSha256"] + .as_str() + .expect("physical fixture digest"); + let basename = artifact["basename"] + .as_str() + .expect("physical fixture basename"); + let content = match basename { + POLICY_BASENAME => b"policy-current".as_slice(), + "PolicyAgent.log.1" => b"policy-rotation-one".as_slice(), + "PolicyAgent.log.2" => b"policy-rotation-two".as_slice(), + _ => panic!("unexpected physical fixture basename"), + }; + assert_eq!( + sha256(content), + expected_digest, + "fixture content and manifest digest disagree for {basename}" + ); let destination = root.join(relative_path);Note that the assertion form does not apply to
reader_rejects_overflowing_physical_byte_totals_before_hashing_evidenceat line 546, which mutatesbytesCopiedafter the fact but leavescontentSha256intact.Also applies to: 181-200
🤖 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 88 - 90, Update the SCCM fixture flow around physical_artifact and write_native_bundle so the supplied content slice is the sole source for both manifest metadata and written evidence bytes; pass it through explicitly instead of reconstructing bytes from the basename. Preserve the overflow test’s ability to mutate bytesCopied after fixture creation without changing contentSha256.
545-558: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an over-limit test that does not rely on arithmetic overflow.
This test sets
bytesCopiedtou64::MAXon one artifact andmaxBytesPerSourcetou64::MAX. The rejection comes from thechecked_addoverflow at line 161 ofsrc-tauri/src/sccm/manifest.rs, not from a client-owned ceiling. The test name claims the check happens "before hashing evidence", but the only reason nothing is hashed is the overflow of the second artifact's bytes.A manifest that declares
maxBytesPerSource: u64::MAXwith a truthful largebytesCopiedpassesvalidate_physical_source_limits.validate_evidence_filethen hashes the whole file. This is the unbounded-admission issue raised onsrc-tauri/src/sccm/manifest.rslines 137-175.After the reader enforces fixed client-owned limits, add two tests: one artifact exactly at the client-owned per-artifact ceiling that is accepted, and one artifact one byte over that ceiling that is rejected without an evidence read.
🤖 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 545 - 558, Replace the overflow-based test around read_sccm_manifest_or_legacy with coverage for fixed client-owned limits: add one case where an artifact’s bytesCopied equals the per-artifact ceiling and is accepted, and another where it exceeds that ceiling by one byte and is rejected before validate_evidence_file reads or hashes evidence. Use truthful non-overflowing byte totals and configure maxBytesPerSource independently so the tests verify admission limits rather than checked_add overflow.src-tauri/src/sccm/manifest.rs (3)
493-506: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated provenance comparison.
validate_bound_provenanceandvalidate_enumeration_provenanceend with the same three comparisons againstcmtraceopen.source.sha256.v1:,sha256:, andcmtraceopen.lineage.sha256.v1:. Only the expected source digest and the error string differ. A shared helper keeps the prefixes defined once, so a future prefix change cannot update one site and miss the other.♻️ Proposed refactor
+fn match_provenance_triple( + source_handle: &str, + fingerprint: &str, + lineage: &str, + expected_source_digest: &str, + error_message: &str, +) -> Result<(), AppError> { + let expected_lineage = sha256_bytes(format!("lineage:v1:{expected_source_digest}").as_bytes()); + if source_handle.strip_prefix("cmtraceopen.source.sha256.v1:") != Some(expected_source_digest) + || fingerprint.strip_prefix("sha256:") != Some(expected_source_digest) + || lineage.strip_prefix("cmtraceopen.lineage.sha256.v1:") != Some(expected_lineage.as_str()) + { + return Err(AppError::InvalidInput(error_message.to_owned())); + } + Ok(()) +}Then call it from both validators, and from
validate_capture_gap, which repeats the same block at lines 459-481.Also applies to: 522-539
🤖 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 493 - 506, Extract the repeated source-handle, fingerprint, and lineage prefix comparisons into a shared helper in the provenance validation code, keeping the existing malformed-provenance error behavior configurable or preserved per caller. Replace the duplicated comparison blocks in validate_enumeration_provenance, validate_capture_gap, and validate_bound_provenance with calls to that helper, passing each validator’s expected source digest and error context.
823-826: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify the subsumed match arms and document the
collectedmapping.At line 825,
Some("Failed")is subsumed by the followingSome(_)arm. At line 837,Some("collected")andSome("failed")are subsumed the same way. All alternatives produceFailedUnknownDetail, so the extra patterns have no effect.The mapping of
"collected"toFailedUnknownDetailis deliberate, because the legacy manifest carries no verifiable evidence binding. That intent is not obvious from the code. Replace the subsumed arms with a comment.♻️ Proposed refactor
let state = match artifact.get("status").and_then(Value::as_str) { Some("missing") => SccmManifestSourceState::Absent, - Some("collected") | Some("failed") | Some(_) | None => { - SccmManifestSourceState::FailedUnknownDetail - } + // Legacy manifests carry no verifiable evidence binding, so even a + // "collected" status cannot be projected as captured coverage. + _ => SccmManifestSourceState::FailedUnknownDetail, };Also applies to: 835-840
🤖 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 823 - 826, In the status mapping match near the `gap.get("status")` logic, remove the redundant `Some("Failed")` pattern and retain a single fallback arm for all other values. Apply the same simplification to the later mapping around `"collected"` and `"failed"`, adding a concise comment documenting why `"collected"` intentionally maps to `FailedUnknownDetail` due to the legacy manifest lacking verifiable evidence binding.
61-64: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRemove the duplicated structure validation.
validate_native_manifestcallsvalidate_native_manifest_structureat line 126, then callsmanifest_to_client_intake_bundleat line 127, which callsvalidate_native_manifest_structureagain at line 64. Every native read validates the full artifact set twice. The validation recomputes SHA-256 catalog and marker identities for up toMAX_SCCM_MANIFEST_ARTIFACTSentries, so the duplicate pass doubles the hashing cost on the read path.Keep the structure check in one place.
manifest_to_client_intake_bundleis also called directly fromread_sccm_client_intake_bundle, so the simplest fix is to drop the call at line 126 and rely on the one insidemanifest_to_client_intake_bundle.♻️ Proposed refactor
fn validate_native_manifest( bundle_root: &VerifiedBundleRoot, manifest: &SccmBundleManifestV1, ) -> Result<(), AppError> { - validate_native_manifest_structure(manifest)?; manifest_to_client_intake_bundle(manifest)?; validate_physical_source_limits(manifest)?;Also applies to: 122-128
🤖 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 61 - 64, Remove the redundant validate_native_manifest_structure call from validate_native_manifest, while retaining the check inside manifest_to_client_intake_bundle so direct callers such as read_sccm_client_intake_bundle remain validated exactly once.
🤖 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 `@src-tauri/src/sccm/manifest.rs`:
- Around line 137-175: The manifest validation currently trusts unbounded
manifest-declared byte caps. In src-tauri/src/sccm/manifest.rs#L137-L175, define
client-owned per-artifact and aggregate physical-byte constants, enforce both in
validate_physical_source_limits before evidence files are opened, and use the
manifest caps only as additional upper bounds. In
src-tauri/tests/sccm_client_manifest.rs#L545-L558, add coverage confirming an
artifact exactly at the per-artifact ceiling is accepted and one byte over is
rejected without reading evidence.
- Around line 783-792: Update the catalog_id initialization in the legacy
ccmsetup artifact construction to call catalog_entry_id with
LEGACY_CCMSETUP_BASENAME instead of LEGACY_CCMSETUP_GROUP, while leaving the
expected_marker_artifact_id flow unchanged.
In `@src-tauri/src/sccm/private_fs.rs`:
- Around line 830-882: Update the Windows test fixtures for
missing_final_component_preserves_not_found_for_legacy_fallback,
relative_component_rejects_alternate_data_streams, and
verified_root_keeps_reading_the_original_directory_after_root_replacement to
apply an explicit restrictive DACL to each created bundle directory before
calling verify_bundle_root. Match the Unix fixture’s restrictive permissions so
the tests do not depend on ambient temporary-directory ACLs.
- Around line 798-810: Both swap tests leave the thread-local open-component
hook installed when open_relative_file panics; add a shared
OpenComponentHookGuard near the hook helpers whose install method registers the
hook and whose Drop implementation clears it. Use this guard at
src-tauri/src/sccm/private_fs.rs lines 798-810 and 900-912, removing each
explicit set_open_component_hook(None) reset so cleanup occurs on every exit
path.
---
Nitpick comments:
In `@src-tauri/src/sccm/contract.rs`:
- Around line 394-399: Update sha256_bytes to construct the digest’s hexadecimal
representation in one preallocated String rather than mapping each byte through
format!, or reuse an existing hex-encoding dependency. Preserve the current
lowercase two-character-per-byte output and continue using Sha256::digest.
In `@src-tauri/src/sccm/manifest.rs`:
- Around line 493-506: Extract the repeated source-handle, fingerprint, and
lineage prefix comparisons into a shared helper in the provenance validation
code, keeping the existing malformed-provenance error behavior configurable or
preserved per caller. Replace the duplicated comparison blocks in
validate_enumeration_provenance, validate_capture_gap, and
validate_bound_provenance with calls to that helper, passing each validator’s
expected source digest and error context.
- Around line 823-826: In the status mapping match near the `gap.get("status")`
logic, remove the redundant `Some("Failed")` pattern and retain a single
fallback arm for all other values. Apply the same simplification to the later
mapping around `"collected"` and `"failed"`, adding a concise comment
documenting why `"collected"` intentionally maps to `FailedUnknownDetail` due to
the legacy manifest lacking verifiable evidence binding.
- Around line 61-64: Remove the redundant validate_native_manifest_structure
call from validate_native_manifest, while retaining the check inside
manifest_to_client_intake_bundle so direct callers such as
read_sccm_client_intake_bundle remain validated exactly once.
In `@src-tauri/src/sccm/private_fs.rs`:
- Line 120: Replace every Windows-specific #[cfg(windows)] attribute in
private_fs.rs with #[cfg(target_os = "windows")], including the gated items
around the identified locations, while leaving the surrounding implementations
unchanged.
- Around line 668-675: Update the reparse-point checks in is_reparse_point and
the code around FILE_FLAG_OPEN_REPARSE_POINT to reuse the existing windows crate
constants, importing them where needed and removing the local constant
definitions. Preserve the current flag checks and behavior.
- Around line 387-403: Clear O_NONBLOCK on the final file descriptor opened by
the traversal function before returning it, matching the behavior of
open_file_no_follow. Extract the existing fcntl-based reset into a
clear_nonblock helper and reuse it from both paths, propagating any fcntl errors
while preserving all other descriptor flags.
- Around line 329-330: Update open_file_no_follow and its platform-specific
definitions so they are compiled only for the Unix test configuration where the
function is used, or remove the unused Windows and fallback variants. Preserve
the Unix test caller and avoid retaining unused release-build definitions.
In `@src-tauri/tests/sccm_client_manifest.rs`:
- Around line 343-353: Rename
reader_rejects_duplicate_artifact_ids_and_relative_paths to reflect that the
fixture only verifies duplicate artifact ID rejection, since identical
physical_artifact values fail before relative-path validation. Keep the existing
assertions and test behavior unchanged.
- Around line 113-121: Add a test in the SCCM client manifest test coverage that
constructs a captured artifact with "fragmentComplete": true, then assert the
projected fragment_complete value is Some(true). Reuse the existing
captured-artifact fixture or test setup and target the manifest projection path
exercised by the existing tests.
- Around line 560-574: Add a Windows-gated counterpart to
reader_rejects_hard_linked_physical_evidence that creates a duplicate file name
with std::fs::hard_link in the same temporary bundle, invokes
read_sccm_manifest_or_legacy, and asserts the same safe-opening rejection. Keep
the existing Unix test unchanged and use #[cfg(windows)] for the new test.
- Around line 402-435: Strengthen both symlink tests by asserting the specific
rejection errors, not only the absence of paths. In
reader_rejects_a_symlinked_bundle_root_without_following_it, require the error
text to include “not private” alongside the existing path assertion; in
reader_opens_manifest_without_following_a_symlink, assert the expected
no-follow/open error text so valid JSON from the outside file cannot be mistaken
for a schema failure.
- Around line 88-90: Update the SCCM fixture flow around physical_artifact and
write_native_bundle so the supplied content slice is the sole source for both
manifest metadata and written evidence bytes; pass it through explicitly instead
of reconstructing bytes from the basename. Preserve the overflow test’s ability
to mutate bytesCopied after fixture creation without changing contentSha256.
- Around line 545-558: Replace the overflow-based test around
read_sccm_manifest_or_legacy with coverage for fixed client-owned limits: add
one case where an artifact’s bytesCopied equals the per-artifact ceiling and is
accepted, and another where it exceeds that ceiling by one byte and is rejected
before validate_evidence_file reads or hashes evidence. Use truthful
non-overflowing byte totals and configure maxBytesPerSource independently so the
tests verify admission limits rather than checked_add overflow.
🪄 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: 30408aa0-9a73-45d7-934c-3dccf8db6e82
📒 Files selected for processing (7)
src-tauri/Cargo.tomlsrc-tauri/src/lib.rssrc-tauri/src/sccm/contract.rssrc-tauri/src/sccm/manifest.rssrc-tauri/src/sccm/mod.rssrc-tauri/src/sccm/private_fs.rssrc-tauri/tests/sccm_client_manifest.rs
…r SetSecurityInfo make_private_directory was opening the bundle fixture directory with .read(true) (GENERIC_READ), which includes READ_CONTROL but not WRITE_DAC. SetSecurityInfo with DACL_SECURITY_INFORMATION requires WRITE_DAC on the handle; without it the call returns ERROR_ACCESS_DENIED (WIN32_ERROR(5)), failing the four windows_tests that call make_private_directory on GitHub Actions hosted Windows runners. Switch to .access_mode(READ_CONTROL.0 | WRITE_DAC.0) so both the owner query (GetSecurityInfo) and the DACL write (SetSecurityInfo) succeed on a handle with exactly the rights they need.
Reviewed
All four sites are consistently gated:
On Windows test builds none of these symbols exist, eliminating the dead-code Clippy warning. The behavioral regression on Unix (exactly one projection pass through Windows DACL fixture fix (
CI The |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src-tauri/src/sccm/manifest.rs (1)
53-59: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winLegacy fallback re-verifies the bundle root and re-parses the manifest.
ValidatedManifestRead::Legacystores only the typedSccmBundleManifestV1, not theVerifiedBundleRootused to read it. Inread_sccm_client_intake_bundle, theLegacybranch callsverify_bundle_root(bundle_root)again, andread_legacy_client_intake_bundlethen callsread_legacy_valueto re-open and re-parsemanifest.jsonas a separateserde_json::Value. The Native branch avoids this by reusing its storedintake_bundle; the Legacy branch does not get the same treatment.Every legacy-manifest intake read now performs the bundle-root verification (including the Windows owner/SID/ACL enumeration) and the manifest file open-and-parse twice. Carry the
VerifiedBundleRoot(and, if convenient, the raw legacyValue) insideValidatedManifestRead::Legacysoread_sccm_client_intake_bundlereuses it instead of re-deriving it.♻️ Sketch of avoiding the duplicate root verification
enum ValidatedManifestRead { Native { manifest: SccmBundleManifestV1, intake_bundle: SccmClientIntakeBundle, }, - Legacy(SccmBundleManifestV1), + Legacy { + verified_root: VerifiedBundleRoot, + manifest: SccmBundleManifestV1, + }, }pub fn read_sccm_client_intake_bundle( bundle_root: &Path, ) -> Result<SccmClientIntakeBundle, AppError> { match read_validated_manifest_or_legacy(bundle_root)? { ValidatedManifestRead::Native { intake_bundle, .. } => Ok(intake_bundle), - ValidatedManifestRead::Legacy(_) => { - let verified_root = verify_bundle_root(bundle_root)?; - read_legacy_client_intake_bundle(&verified_root) - } + ValidatedManifestRead::Legacy { verified_root, .. } => { + read_legacy_client_intake_bundle(&verified_root) + } } }Also applies to: 150-160
🤖 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 53 - 59, Update ValidatedManifestRead::Legacy and the read_sccm_client_intake_bundle flow to retain and reuse the VerifiedBundleRoot produced during initial validation, along with the already parsed legacy manifest Value if available. Change read_legacy_client_intake_bundle to consume these stored values instead of calling verify_bundle_root or read_legacy_value again, while preserving the existing Native and Legacy intake behavior.
🤖 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/manifest.rs`:
- Around line 53-59: Update ValidatedManifestRead::Legacy and the
read_sccm_client_intake_bundle flow to retain and reuse the VerifiedBundleRoot
produced during initial validation, along with the already parsed legacy
manifest Value if available. Change read_legacy_client_intake_bundle to consume
these stored values instead of calling verify_bundle_root or read_legacy_value
again, while preserving the existing Native and Legacy intake behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4402fe74-514d-4ab6-8056-a9e2d7f67229
📒 Files selected for processing (3)
src-tauri/src/sccm/manifest.rssrc-tauri/src/sccm/private_fs.rssrc-tauri/tests/sccm_client_manifest.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src-tauri/tests/sccm_client_manifest.rs
|
Exact-head review request for Copilot's Windows fixture-rights correction is retained but was not accepted on authorship alone. Independent full-range and Windows-specific audits confirmed the prior hosted RED: four Windows private-filesystem tests failed at This follow-up strengthens the source-file-cap ordering test by deleting an evidence fragment before reader admission. A local mutation that moved evidence validation ahead of the cap check made the focused test fail; restoring the production order made it pass. The full native manifest suite is 21/21, changed-file Rust formatting and @coderabbitai review Please review the complete current range at this exact head. Hosted Windows workspace tests and strict Clippy remain mandatory; this draft must not merge on the earlier-head approvals. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
head_short='7d336185'
head="$(git rev-parse "${head_short}^{commit}")"
printf '=== checked-out commit ===\n'
git rev-parse HEAD
printf '\n=== requested commit ===\n%s\n' "$head"
printf '\n=== requested commit metadata ===\n'
git show -s --format='%H%n%P%n%s' "$head"
printf '\n=== remotes and candidate base refs ===\n'
git remote -v || true
git branch -a --contains "$head" || true
for ref in origin/main origin/master main master; do
if git rev-parse --verify -q "$ref" >/dev/null; then
printf '%s %s\n' "$ref" "$(git rev-parse "$ref")"
fi
done
base=''
for ref in origin/main origin/master main master; do
if git rev-parse --verify -q "$ref" >/dev/null; then
candidate="$(git merge-base "$ref" "$head")"
if [ -z "$base" ] || [ "$(git rev-list --count "$candidate..$head")" -gt "$(git rev-list --count "$base..$head")" ]; then
base="$candidate"
fi
fi
done
if [ -z "$base" ]; then
base="$(git rev-parse "${head}^")"
fi
printf '\n=== selected review base ===\n%s\n' "$base"
printf '\n=== commits in reviewed range ===\n'
git log --oneline --reverse "${base}..${head}"
printf '\n=== changed files ===\n'
git diff --name-status "$base" "$head"
printf '\n=== diff stat ===\n'
git diff --stat "$base" "$head"
printf '\n=== Rust outlines for SCCM changes ===\n'
for path in src-tauri/src/sccm/contract.rs src-tauri/src/sccm/manifest.rs src-tauri/src/sccm/private_fs.rs src-tauri/src/sccm/mod.rs src-tauri/tests/sccm_client_manifest.rs; do
if [ -f "$path" ]; then
printf '\n--- %s ---\n' "$path"
ast-grep outline "$path" --items all || true
fi
done
printf '\n=== exact head delta ===\n'
parent="$(git rev-parse "${head}^")"
git diff --unified=60 "$parent" "$head" -- \
src-tauri/src/sccm/private_fs.rs \
src-tauri/src/sccm/manifest.rs \
src-tauri/tests/sccm_client_manifest.rsLength of output: 196440 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '=== final two commits ===\n'
git diff --unified=100 c4e4c9baeffbf2eede6ed367870f5d18b57e6e21^ 7d336185ddf46e27489bf573b3ce734f99dad45a -- \
src-tauri/src/sccm/private_fs.rs \
src-tauri/src/sccm/manifest.rs \
src-tauri/tests/sccm_client_manifest.rs
printf '\n=== native manifest admission and evidence paths ===\n'
nl -ba src-tauri/src/sccm/manifest.rs | sed -n \
'35,239p;240,390p;604,730p;731,1004p;1005,1065p'
printf '\n=== Unix handle-relative traversal ===\n'
nl -ba src-tauri/src/sccm/private_fs.rs | sed -n \
'1,121p;330,428p;549,583p;654,733p;734,859p'
printf '\n=== Windows root ACL, handle-relative traversal, and tests ===\n'
nl -ba src-tauri/src/sccm/private_fs.rs | sed -n \
'122,329p;429,548p;584,653p;860,1085p'
printf '\n=== all native reader open/read calls and AppError construction ===\n'
rg -n -C 4 \
'\b(open_relative_file|open_file_no_follow|read_bounded_file|sha256_exact_file|File::open|fs::read|AppError::|map_err|raw_os_error|NTSTATUS|ntstatus|ERROR_)' \
src-tauri/src/sccm/manifest.rs \
src-tauri/src/sccm/private_fs.rs \
src-tauri/tests/sccm_client_manifest.rs
printf '\n=== all calls to validated manifest and evidence validation ===\n'
rg -n -C 4 \
'\b(read_validated_manifest_or_legacy|validate_native_manifest|validate_physical_source_limits|validate_evidence_file|read_sccm_manifest_or_legacy|read_sccm_client_intake_bundle)\s*\(' \
src-tauri
printf '\n=== Windows CI workflow SCCM and Rust checks ===\n'
rg -n -i -C 8 'windows|clippy|sccm|cargo test|rustfmt' .github/workflows/cmtrace-ci.ymlLength of output: 1674
I confirmed the requested head in the first inspection. I could not verify the Windows fixture-rights change, the cap-before-evidence ordering, the Windows traversal code, or the required CI gates. Please retry the review when repository access is available. You are interacting with an AI system. |
|
@coderabbitai review Please review the current exact head |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
head='bcb4ac54fff4912b55396822514ca943422fb4ac'
git rev-parse --verify "${head}^{commit}"
printf 'checked_out=%s\n' "$(git rev-parse HEAD)"
printf 'requested_head=%s\n' "$(git rev-parse "${head}^{commit}")"
git show -s --format='subject=%s%nparents=%P' "$head"
base=''
for ref in origin/main origin/master main master; do
if git rev-parse --verify -q "$ref" >/dev/null; then
candidate="$(git merge-base "$ref" "$head")"
if [ -z "$base" ] || [ "$(git rev-list --count "${candidate}..${head}")" -gt "$(git rev-list --count "${base}..${head}")" ]; then
base="$candidate"
fi
fi
done
if [ -z "$base" ]; then
base="$(git rev-parse "${head}^")"
fi
printf 'review_base=%s\n' "$base"
printf '\n=== commits ===\n'
git log --oneline --reverse "${base}..${head}"
printf '\n=== changed files ===\n'
git diff --name-status "$base" "$head"
printf '\n=== diff stat ===\n'
git diff --stat "$base" "$head"
printf '\n=== outlines ===\n'
for path in \
src-tauri/src/sccm/contract.rs \
src-tauri/src/sccm/manifest.rs \
src-tauri/src/sccm/private_fs.rs \
src-tauri/src/sccm/mod.rs \
src-tauri/tests/sccm_client_manifest.rs
do
if [ -f "$path" ]; then
printf '\n--- %s ---\n' "$path"
ast-grep outline "$path" --items all || true
fi
done
printf '\n=== manifest admission, projection, and error surfaces ===\n'
rg -n -C 5 \
'\b(read_sccm_manifest_or_legacy|read_sccm_client_intake_bundle|read_validated_manifest_or_legacy|validate_native_manifest|validate_physical_source_limits|validate_evidence_file|read_bounded_file|sha256_exact_file|manifest_to_client_intake_bundle)\s*\(|MAX_.*(BYTE|SIZE|ARTIFACT|MANIFEST)|AppError::|map_err|display|legacy|manifest\.json' \
src-tauri/src/sccm/manifest.rs \
src-tauri/src/sccm/contract.rs \
src-tauri/tests/sccm_client_manifest.rs || true
printf '\n=== filesystem traversal and Windows status mapping ===\n'
rg -n -C 6 \
'\b(verify_bundle_root|open_relative_file|open_file_no_follow|NtCreateFile|NTSTATUS|ntstatus|STATUS_|RtlNtStatusToDosError|raw_os_error|reparse|hard.?link|nofollow|NOFOLLOW|FILE_FLAG|WRITE_DAC|READ_CONTROL)\b' \
src-tauri/src/sccm/private_fs.rs \
src-tauri/tests/sccm_client_manifest.rs || true
printf '\n=== source ranges ===\n'
nl -ba src-tauri/src/sccm/manifest.rs | sed -n '1,280p;540,780p;940,1100p'
nl -ba src-tauri/src/sccm/private_fs.rs | sed -n '1,360p;360,760p;760,1120p'
nl -ba src-tauri/tests/sccm_client_manifest.rs | sed -n '1,760p'
printf '\n=== full range diff for reviewed files ===\n'
git diff --unified=35 "$base" "$head" -- \
src-tauri/Cargo.toml \
src-tauri/src/lib.rs \
src-tauri/src/sccm/contract.rs \
src-tauri/src/sccm/manifest.rs \
src-tauri/src/sccm/private_fs.rs \
src-tauri/src/sccm/mod.rs \
src-tauri/tests/sccm_client_manifest.rs
printf '\n=== workflow gates ===\n'
if [ -f .github/workflows/cmtrace-ci.yml ]; then
rg -n -i -C 5 'windows|clippy|rustfmt|cargo test|sccm' .github/workflows/cmtrace-ci.yml || true
fiLength of output: 3075
I could not verify the requested commit, inspect the full PR range, or assess the SCCM implementation. Please retry the review when repository access is available. You are interacting with an AI system. |
|
@coderabbitai review Retry the full review at exact head |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
head='bcb4ac54fff4912b55396822514ca943422fb4ac'
base_ref='codex/parser-family-skeleton'
# Fetch only the requested review targets when they are absent locally.
if ! git rev-parse --verify -q "${head}^{commit}" >/dev/null; then
git fetch --no-tags origin "$head"
fi
if ! git rev-parse --verify -q "$base_ref" >/dev/null; then
git fetch --no-tags origin "$base_ref:$base_ref"
fi
base="$(git merge-base "$base_ref" "$head")"
printf 'checked_out=%s\n' "$(git rev-parse HEAD)"
printf 'requested_head=%s\n' "$(git rev-parse "$head")"
printf 'comparison_ref=%s\n' "$(git rev-parse "$base_ref")"
printf 'review_base=%s\n' "$base"
printf '\n=== reviewed commits ===\n'
git log --oneline --reverse "${base}..${head}"
printf '\n=== changed files ===\n'
git diff --name-status "$base" "$head"
printf '\n=== diff stat ===\n'
git diff --stat "$base" "$head"
printf '\n=== Rust outlines ===\n'
git diff --name-only "$base" "$head" | while IFS= read -r path; do
case "$path" in
*.rs)
printf '\n--- %s ---\n' "$path"
ast-grep outline "$path" --items all || true
;;
esac
done
printf '\n=== focused symbols ===\n'
rg -n -C 5 \
'\b(read_sccm_manifest_or_legacy|read_sccm_client_intake_bundle|read_validated_manifest_or_legacy|validate_native_manifest|validate_physical_source_limits|validate_evidence_file|read_bounded_file|sha256_exact_file|manifest_to_client_intake_bundle|verify_bundle_root|open_relative_file|open_file_no_follow|NtCreateFile|RtlNtStatusToDosError)\s*\(|MAX_.*(BYTE|SIZE|ARTIFACT|MANIFEST)|NTSTATUS|reparse|hard.?link|legacy|AppError::' \
src-tauri/src/sccm \
src-tauri/tests/sccm_client_manifest.rs 2>/dev/null || true
printf '\n=== full review diff ===\n'
git diff --unified=50 "$base" "$head" -- \
src-tauri/Cargo.toml \
src-tauri/src/lib.rs \
src-tauri/src/sccm/contract.rs \
src-tauri/src/sccm/manifest.rs \
src-tauri/src/sccm/private_fs.rs \
src-tauri/src/sccm/mod.rs \
src-tauri/tests/sccm_client_manifest.rsLength of output: 100235 🏁 Script executed: #!/bin/bash
set -euo pipefail
head='bcb4ac54fff4912b55396822514ca943422fb4ac'
base="$(git merge-base codex/parser-family-skeleton "$head")"
printf '=== range identity ===\n'
printf 'base=%s\nhead=%s\n' "$base" "$(git rev-parse "$head")"
git diff --name-status "$base" "$head"
printf '\n=== manifest API and admission order ===\n'
ast-grep outline src-tauri/src/sccm/manifest.rs --items all
rg -n -C 12 \
'^(pub )?fn (read_sccm_manifest_or_legacy|read_sccm_client_intake_bundle|read_validated_manifest_or_legacy|validate_native_manifest|validate_physical_source_limits|validate_evidence_file|read_bounded_file|sha256_exact_file)|\b(validate_physical_source_limits|validate_evidence_file|open_relative_file)\s*\(' \
src-tauri/src/sccm/manifest.rs
sed -n '1,260p' src-tauri/src/sccm/manifest.rs
sed -n '540,730p' src-tauri/src/sccm/manifest.rs
printf '\n=== contract decoding bounds and projection ===\n'
ast-grep outline src-tauri/src/sccm/contract.rs --items all
rg -n -C 10 \
'MAX_|Deserialize|deserialize|unknown_fields|deny_unknown|manifest_to_client_intake_bundle|assess_client_intake|AppError::' \
src-tauri/src/sccm/contract.rs src-tauri/src/sccm/manifest.rs
sed -n '1,260p' src-tauri/src/sccm/contract.rs
printf '\n=== Windows traversal and error mapping ===\n'
rg -n -C 12 \
'fn (verify_bundle_root|open_relative_file_no_follow|require_real_windows_directory|require_real_windows_file|windows_file_information)|NtCreateFile|RtlNtStatusToDosError|FILE_OPEN_REPARSE_POINT|FILE_SYNCHRONOUS_IO_NONALERT|from_raw_os_error|ErrorKind::NotFound' \
src-tauri/src/sccm/private_fs.rs
sed -n '410,680p' src-tauri/src/sccm/private_fs.rs
printf '\n=== all public reader error construction ===\n'
rg -n -C 3 'AppError::(InvalidInput|Parse)|map_err' src-tauri/src/sccm/manifest.rs src-tauri/src/sccm/private_fs.rs
printf '\n=== parser contract consumers ===\n'
rg -n -C 5 \
'\b(SccmClientIntakeBundle|SccmClientIntakeArtifact|SccmClientCaptureGap|assess_client_intake)\b' \
crates/cmtraceopen-parser/srcLength of output: 201794 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '=== complete native manifest deserializer ===\n'
sed -n '229,393p' src-tauri/src/sccm/contract.rs
printf '\n=== Cargo feature and integration-test declarations ===\n'
sed -n '1,260p' src-tauri/Cargo.toml
rg -n -C 5 'sccm-diagnostics|sccm_client_manifest|cfg\(feature' \
src-tauri/Cargo.toml src-tauri/src/lib.rs src-tauri/tests/sccm_client_manifest.rs
printf '\n=== all SCCM public API exports and call sites ===\n'
rg -n -C 4 \
'\b(read_sccm_manifest_or_legacy|read_sccm_client_intake_bundle|SccmBundleManifestV1)\b' \
src-tauri crates
printf '\n=== all Windows-only dependencies referenced by the new traversal ===\n'
rg -n -C 4 \
'Win32_System_IO|Wdk_Foundation|Wdk_Storage_FileSystem|Win32_Security|Win32_Storage_FileSystem' \
src-tauri/Cargo.tomlLength of output: 58868
I found no actionable findings. The review covered manifest decode bounds, reader-owned physical byte limits before evidence I/O, legacy fallback behavior, Windows handle-relative traversal, NTSTATUS-to-Win32 mapping, reparse-point and hard-link rejection, and privacy-sensitive error paths. I did not execute repository code in this review environment. You are interacting with an AI system. |
|
@coderabbitai review Please review exact head The prior full-range local review found two valid test-coverage concerns. Commit Local focused tests, strict app Clippy, scoped formatting/diff, and exact |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
head='53c49338d326f2deab1f93165233f94da09c0ab2'
base_ref='codex/parser-family-skeleton'
if ! git rev-parse --verify -q "${head}^{commit}" >/dev/null; then
git fetch --no-tags origin "$head"
fi
if ! git rev-parse --verify -q "${base_ref}^{commit}" >/dev/null; then
git fetch --no-tags origin "${base_ref}:${base_ref}"
fi
base="$(git merge-base "$base_ref" "$head")"
parent="$(git rev-parse "${head}^")"
printf '=== identities ===\n'
printf 'head=%s\nbase_ref=%s\nbase=%s\nparent=%s\n' \
"$(git rev-parse "$head")" "$(git rev-parse "$base_ref")" "$base" "$parent"
git show -s --format='subject=%s%nparents=%P' "$head"
printf '\n=== full range commits and files ===\n'
git log --oneline --reverse "${base}..${head}"
git diff --name-status "$base" "$head"
printf '\n=== exact head delta ===\n'
git diff --unified=80 "$parent" "$head" -- \
src-tauri/src/sccm/manifest.rs \
src-tauri/tests/sccm_client_manifest.rs \
src-tauri/src/sccm/private_fs.rs
printf '\n=== preceding DACL fixture delta ===\n'
git diff --unified=80 bcb4ac54fff4912b55396822514ca943422fb4ac 244448b7 -- \
src-tauri/src/sccm/private_fs.rs \
src-tauri/tests/sccm_client_manifest.rs
printf '\n=== module outlines ===\n'
for path in src-tauri/src/sccm/manifest.rs src-tauri/src/sccm/private_fs.rs src-tauri/src/sccm/contract.rs src-tauri/tests/sccm_client_manifest.rs; do
printf '\n--- %s ---\n' "$path"
ast-grep outline "$path" --items all || true
done
printf '\n=== cap logic, admission ordering, and tests ===\n'
rg -n -C 12 \
'\b(validate_physical_source_limits|validate_evidence_file|read_bounded_file|checked_add|MAX_.*(BYTE|EVIDENCE)|u64::MAX|source.*cap|aggregate)\b' \
src-tauri/src/sccm/manifest.rs \
src-tauri/tests/sccm_client_manifest.rs
printf '\n=== Windows fixture rights and private-fs checks ===\n'
rg -n -C 12 \
'\b(WRITE_DAC|READ_CONTROL|SetSecurityInfo|protected|DACL|hard.?link|reparse|create_.*fixture|verify_bundle_root|open_relative_file|NtCreateFile|RtlNtStatusToDosError)\b' \
src-tauri/src/sccm/private_fs.rs \
src-tauri/tests/sccm_client_manifest.rs
printf '\n=== focused implementation ranges ===\n'
nl -ba src-tauri/src/sccm/manifest.rs | sed -n '1,260p;560,750p;960,1100p'
nl -ba src-tauri/src/sccm/private_fs.rs | sed -n '1,360p;400,760p;820,1120p'
nl -ba src-tauri/tests/sccm_client_manifest.rs | sed -n '1,760p'Length of output: 101881 🏁 Script executed: #!/bin/bash
set -euo pipefail
head='53c49338d326f2deab1f93165233f94da09c0ab2'
base="$(git merge-base codex/parser-family-skeleton "$head")"
printf '=== whitespace errors ===\n'
git diff --check "$base" "$head"
printf '\n=== manifest reader, validation, evidence, and legacy paths ===\n'
sed -n '1,245p' src-tauri/src/sccm/manifest.rs
sed -n '240,730p' src-tauri/src/sccm/manifest.rs
sed -n '730,1010p' src-tauri/src/sccm/manifest.rs
printf '\n=== bounded manifest contract decoder ===\n'
sed -n '1,395p' src-tauri/src/sccm/contract.rs
printf '\n=== Windows root verification and handle-relative traversal ===\n'
sed -n '120,335p' src-tauri/src/sccm/private_fs.rs
sed -n '429,670p' src-tauri/src/sccm/private_fs.rs
printf '\n=== feature gates and public SCCM API ===\n'
rg -n -C 5 'sccm-diagnostics|sccm_client_manifest|windows.*features|pub mod sccm' \
src-tauri/Cargo.toml src-tauri/src/lib.rs src-tauri/src/sccm/mod.rs
printf '\n=== all native reader call sites ===\n'
rg -n -C 4 \
'\b(read_sccm_manifest_or_legacy|read_sccm_client_intake_bundle|manifest_to_client_intake_bundle|validate_native_manifest)\s*\(' \
src-tauri crates
printf '\n=== Windows API feature imports and referenced symbols ===\n'
rg -n -C 3 \
'Win32_System_IO|Wdk_Foundation|Wdk_Storage_FileSystem|NtCreateFile|RtlNtStatusToDosError|IO_STATUS_BLOCK|OBJECT_ATTRIBUTES' \
src-tauri/Cargo.toml src-tauri/src/sccm/private_fs.rs
printf '\n=== complete review diff, compact form ===\n'
git diff --unified=12 "$base" "$head" -- \
src-tauri/Cargo.toml \
src-tauri/src/lib.rs \
src-tauri/src/sccm/contract.rs \
src-tauri/src/sccm/manifest.rs \
src-tauri/src/sccm/private_fs.rs \
src-tauri/src/sccm/mod.rs \
src-tauri/tests/sccm_client_manifest.rsLength of output: 236828
I found no actionable findings. The full reviewed range is The new The Windows hard-link and reparse tests now use the protected owner-only DACL helper. The Windows traversal retains handle-relative behavior, preserves I did not execute tests. Hosted Windows runtime and full CI remain required. You are interacting with an AI system. |
Scope
Advances #319 under epic #317 with the first native client-intake stack only:
manifest.jsonfallback without public path disclosure;This stack does not add source discovery, collection, writing, publication, or a public capture command. It does not change the pure parser's platform boundaries.
Dependency and acceptance state
codex/parser-family-skeletonintegration commit26a9a5ee6397696475b3ea709d044f16aa8104fd.84079b8c30bd1ab52d3c7711ea50ee58a92276b2.ErrorKind::Other; the RED/GREEN correction isc3f1c949/84079b8c.Verification
cargo +1.88.0 test --locked -p cmtrace-open --test sccm_client_manifest --features sccm-diagnostics— 18/18 pass.cargo +1.88.0 test --locked -p cmtrace-open --features sccm-diagnostics sccm::private_fs— 3/3 non-Windows tests pass; Windows-gated tests await hosted execution.cargo +1.88.0 test --locked -p cmtraceopen-parser— pass.cargo +1.88.0 check --locked -p cmtraceopen-parser --target wasm32-unknown-unknown— pass.cargo +1.88.0 check --locked -p cmtrace-open --all-features— pass.cargo clippy --locked -p cmtrace-open --all-targets --all-features -- -D warnings— pass on the repository stable toolchain.cargo clippy --locked -p cmtraceopen-parser --all-targets --all-features -- -D warnings— pass on the repository stable toolchain.rustfmt --checkandgit diff --check— pass.cargo fmt --all -- --checkremains red only on inherited files outside this seven-file range; no formatting debt is hidden in this slice.Review focus
NtCreateFilerelative-handle semantics, NTSTATUS mapping, owner/DACL checks, reparse/hard-link handling, and legacy fallback.Summary by CodeRabbit
New Features
Bug Fixes