feat(sccm): assess pure client intake coverage - #394
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds public SCCM client intake contracts, source definitions, validation, artifact classification, coverage assessment, deterministic ordering, catalog integration, and fixture-based contract tests. ChangesSCCM Client Intake Feature
Estimated code review effort: 5 (Critical) | ~100 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant assess_client_intake
participant BundleValidation
participant ClientCatalog
participant CoverageAggregation
Caller->>assess_client_intake: SccmClientIntakeBundle
assess_client_intake->>BundleValidation: validate bundle constraints
BundleValidation-->>assess_client_intake: validation result
assess_client_intake->>ClientCatalog: classify basenames and rotations
ClientCatalog-->>assess_client_intake: normalized fragments and unsupported artifacts
assess_client_intake->>CoverageAggregation: aggregate memberships and coverage
CoverageAggregation-->>assess_client_intake: assessment and coverage gaps
assess_client_intake-->>Caller: SccmClientIntakeAssessment
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 |
Pure intake GREEN checkpoint — exact fd405bdThe production pure-parser slice is pushed at fd405bd. Scope and fixture matrix:
TDD evidence: Verification: focused 9/9, spine 136/136, full parser pass, default strict Clippy pass, Rust 1.88 wasm32 pass, TypeScript pass, 12 JSON files pass, scoped Rust 1.88 formatting and diff/status checks pass. Repository-wide fmt still reports only the inherited 17 non-#319 files; the worktree is clean. Dependency/review state: #318 is merged and consumed. Native manifest/discovery and real Windows validation remain unimplemented, so #319 stays open and no live acceptance is claimed. This draft must receive substantive exact-head CodeRabbit plus a different independent exact-head review before any merge decision. |
|
@coderabbitai review Please perform a substantive review of exact head fd405bd. Focus on pure-Rust/wasm isolation, deterministic rotation/collision behavior, coverage-state conservatism, safe provenance, unknown/lookalike handling, and public-contract compatibility. A skipped or rate-limited response will not be counted as approval. |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
expected='fd405bdf4cf527f20d3d778dedc0dc462d2f4229'
actual="$(git rev-parse HEAD)"
printf 'HEAD=%s\nEXPECTED=%s\n' "$actual" "$expected"
git status --short
git diff --name-status "${expected}^" "$expected"
printf '\n--- Changed Rust files ---\n'
git diff --name-only "${expected}^" "$expected" -- '*.rs'
printf '\n--- Intake-related outlines ---\n'
fd -e rs . | rg 'sccm|intake' | while IFS= read -r file; do
echo "### $file"
ast-grep outline "$file" --items all --type function,struct,enum,trait,impl 2>/dev/null || true
done
printf '\n--- Public intake/export references ---\n'
rg -n -C 3 --glob '*.rs' 'pub (mod|use)|\b(intake|Intake|Coverage|Provenance|LocationServices)\b' . || true
printf '\n--- Exact commit diff ---\n'
git diff --find-renames --find-copies --minimal "${expected}^" "$expected" -- '*.rs' '*.json' '*.toml'Length of output: 100785 🏁 Script executed: #!/bin/bash
set -euo pipefail
commit='fd405bdf4cf527f20d3d778dedc0dc462d2f4229'
intake='crates/cmtraceopen-parser/src/sccm/client/intake.rs'
tests='crates/cmtraceopen-parser/tests/sccm_client_intake.rs'
printf '%s\n' '--- Uncommitted intake diff ---'
git diff -- "$intake"
printf '%s\n' '--- Committed intake implementation: declarations and catalog ---'
git show "$commit:$intake" | nl -ba | sed -n '1,370p'
printf '%s\n' '--- Committed intake implementation: validation, projection, ordering ---'
git show "$commit:$intake" | nl -ba | sed -n '371,760p'
printf '%s\n' '--- Client intake tests ---'
nl -ba "$tests" | sed -n '1,460p'
printf '%s\n' '--- SCCM contract types used by intake ---'
rg -n -C 4 --glob '*.rs' \
'enum SccmRotation|struct SccmClientIntakeArtifact|struct SccmClientIntakeBundle|enum SccmCoverageState|struct SccmArtifact|enum SccmRole' \
crates/cmtraceopen-parser/src/sccm
printf '%s\n' '--- All committed intake call sites and construction sites ---'
rg -n -C 3 --glob '*.rs' \
'\b(assess_client_intake|SccmClientIntakeBundle|SccmClientIntakeArtifact|SccmClientUnsupportedArtifact)\b' \
crates/cmtraceopen-parserLength of output: 1594 🏁 Script executed: #!/bin/bash
set -euo pipefail
commit='fd405bdf4cf527f20d3d778dedc0dc462d2f4229'
intake='crates/cmtraceopen-parser/src/sccm/client/intake.rs'
tests='crates/cmtraceopen-parser/tests/sccm_client_intake.rs'
printf '%s\n' '--- Staged intake diff, excluded from exact-head review ---'
git diff --cached -- "$intake"
printf '%s\n' '--- Committed intake implementation: declarations and catalog ---'
git show "$commit:$intake" | sed -n '1,370p'
printf '%s\n' '--- Committed intake implementation: validation, projection, ordering ---'
git show "$commit:$intake" | sed -n '371,760p'
printf '%s\n' '--- Client intake tests ---'
sed -n '1,460p' "$tests"
printf '%s\n' '--- SCCM contract definitions used by intake ---'
rg -n -C 5 --glob '*.rs' \
'enum SccmRotation|struct SccmClientIntakeArtifact|struct SccmClientIntakeBundle|enum SccmCoverageState|struct SccmArtifact|enum SccmRole' \
crates/cmtraceopen-parser/src/sccm
printf '%s\n' '--- Intake API call sites and construction sites ---'
rg -n -C 3 --glob '*.rs' \
'\b(assess_client_intake|SccmClientIntakeBundle|SccmClientIntakeArtifact|SccmClientUnsupportedArtifact)\b' \
crates/cmtraceopen-parserLength of output: 64753 🏁 Script executed: #!/bin/bash
set -euo pipefail
commit='fd405bdf4cf527f20d3d778dedc0dc462d2f4229'
printf '%s\n' '--- Committed rotation and artifact model ---'
git show "$commit:crates/cmtraceopen-parser/src/sccm/models.rs" | sed -n '255,335p'
git show "$commit:crates/cmtraceopen-parser/src/sccm/models.rs" | sed -n '400,440p'
printf '%s\n' '--- All committed rotation validation and naming logic ---'
git grep -n -E 'SccmRotation|Timestamped|is_safe_.*rotation|rotation.*valid|valid.*rotation' "$commit" -- \
'crates/cmtraceopen-parser/src' 'crates/cmtraceopen-parser/tests' || true
printf '%s\n' '--- Exact committed focused-test section with line numbers ---'
git show "$commit:crates/cmtraceopen-parser/tests/sccm_client_intake.rs" \
| grep -n -A 42 -B 4 'malformed_rotation_and_public_provenance_values_fail_closed'
printf '%s\n' '--- Timestamped fixture manifest values ---'
fd 'manifest.json' crates/cmtraceopen-parser/tests/fixtures/sccm/client/intake \
-x sh -c 'echo "--- $1"; grep -n -A4 -B3 "\"timestamped\"" "$1" || true' sh {}
printf '%s\n' '--- Intake public-contract documentation and schema references ---'
rg -n -C 3 --glob '!target/**' \
'SccmClientIntake|client intake|fragmentComplete|pathFingerprint|capturedUtc|timestamped' \
README.md docs crates/cmtraceopen-parser 2>/dev/null || trueLength of output: 201842
I found one blocking issue.
I also confirmed these parts of the requested scope by code inspection:
I did not run tests because the review sandbox does not permit builds or test execution. The identified timestamped-rotation case indicates that the stated focused 9/9 result should be rechecked after the fix. ✅ Action performedReview finished.
|
|
@coderabbitai The reported timestamped-rotation blocker does not reproduce at exact head fd405bd. Focused verification just rerun: cargo test --locked -p cmtraceopen-parser --test sccm_client_intake malformed_rotation_and_public_provenance_values_fail_closed -- --exact Result: 1 passed, 0 failed. The finding missed the fail-closed serializer gate. crates/cmtraceopen-parser/src/sccm/client/intake.rs:373 calls serde_json::to_value on every rotation before matching. crates/cmtraceopen-parser/src/sccm/models.rs:292-295 makes Serialize reject any noncanonical timestamp, including 2026-bad. Therefore assess_client_intake returns InvalidRotation and expected_rotated_name is never reached. The permanent regression is at tests/sccm_client_intake.rs:291-298. No code change was made because the exact head already enforces the requested invariant. Please re-check and acknowledge this finding as not applicable to fd405bd. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
commit='fd405bdf4cf527f20d3d778dedc0dc462d2f4229'
intake='crates/cmtraceopen-parser/src/sccm/client/intake.rs'
models='crates/cmtraceopen-parser/src/sccm/models.rs'
test_file='crates/cmtraceopen-parser/tests/sccm_client_intake.rs'
printf '%s\n' '--- exact HEAD check ---'
git rev-parse "$commit"
printf '%s\n' '--- intake validation and rotated-name paths ---'
git show "$commit:$intake" | nl -ba | \
grep -E -C 12 'fn validate_bundle|serde_json::to_value|expected_rotated_name|InvalidRotation|Timestamped'
printf '%s\n' '--- SccmRotation serialization ---'
git show "$commit:$models" | nl -ba | \
grep -E -C 16 'SccmRotation|Serialize|Timestamped|timestamp'
printf '%s\n' '--- permanent regression test ---'
git show "$commit:$test_file" | nl -ba | \
grep -E -C 18 'malformed_rotation_and_public_provenance_values_fail_closed|2026-bad|InvalidRotation'Length of output: 1353 🏁 Script executed: #!/bin/bash
set -euo pipefail
commit='fd405bdf4cf527f20d3d778dedc0dc462d2f4229'
intake='crates/cmtraceopen-parser/src/sccm/client/intake.rs'
models='crates/cmtraceopen-parser/src/sccm/models.rs'
test_file='crates/cmtraceopen-parser/tests/sccm_client_intake.rs'
printf '%s\n' '--- intake validation and rotated-name paths ---'
git show "$commit:$intake" | \
grep -n -E -C 12 'fn validate_bundle|serde_json::to_value|expected_rotated_name|InvalidRotation|Timestamped' || true
printf '%s\n' '--- SccmRotation serialization ---'
git show "$commit:$models" | \
grep -n -E -C 16 'SccmRotation|Serialize|Timestamped|timestamp' || true
printf '%s\n' '--- permanent regression test ---'
git show "$commit:$test_file" | \
grep -n -E -C 18 'malformed_rotation_and_public_provenance_values_fail_closed|2026-bad|InvalidRotation' || trueLength of output: 22412
In The regression test at 🐇 ✏️ Learnings added
You are interacting with an AI system. |
Independent exact-head review activeI own read-only review of PR #394 at exact
A formal exact-head COMMENTED PASS or BLOCK with executable evidence will follow. PR remains draft/open. |
adamgell
left a comment
adamgell
left a comment
There was a problem hiding this comment.
Independent exact-head review — BLOCK
Reviewed head fd405bd against base a8a367c in a fresh detached, read-only worktree. All four changed files were reviewed. CodeRabbit has substantively reviewed this exact head and formally withdrew its timestamp claim; I independently confirmed malformed numbered and timestamped rotations fail closed.
Blocking finding
[P1] Reject identity-bearing and control-bearing public provenance.
crates/cmtraceopen-parser/src/sccm/client/intake.rs:612-626 does not bound both public provenance fields to a privacy-safe grammar. is_safe_relative_path allows @, whitespace, control characters, Unicode, and arbitrary extra segments. is_safe_path_identity rejects whitespace but does not reject control characters or non-ASCII values. assess_client_intake then copies these strings into the public assessment unchanged.
An external consumer probe against this exact head passed a captured PolicyAgent artifact with relativePath evidence/client-policy-agent/RealUser@example.com/PolicyAgent.log; assessment succeeded and the identity-bearing path appeared verbatim in serialized public JSON. A second probe with pathFingerprint containing an embedded NUL also succeeded and serialized as \u0000. This violates the program privacy contract, the spine rule that raw user identity must never reach public SCCM JSON, and this PR description claim of fail-closed public provenance.
Please add focused RED regressions through assess_client_intake and public serialization for identity-bearing relative paths and control-bearing fingerprints, then validate the approved bundle layout and every emitted provenance segment/fingerprint with a bounded privacy-safe grammar. Preserve collision-safe opaque lineage, but do not accept arbitrary identity-bearing path segments.
Verified green behavior
- Focused client intake: 9/9 passed.
- SCCM spine: 136/136 passed.
- Full cmtraceopen-parser suite: 738 tests passed.
- Strict Clippy: passed.
- Rust 1.88 wasm32-unknown-unknown check: passed.
- Changed-file Rust 1.88 rustfmt, git diff --check, 12 intake JSON parses, detached status: passed/clean.
- Global cargo fmt --check --all still reports the inherited 17-file non-#319 baseline; none of the four changed files differs.
- Exact 11-group catalog, current → .lo_ → numbered → timestamped order, malformed rotation rejection, case-insensitive collision rejection across supported/unsupported inputs, deterministic input reorder, most-limiting coverage, and one physical LocationServices projection into two consumers all passed adversarial probes.
- Production additions contain no native I/O, Tauri, Windows, network, registry/WMI, database, workflow diagnosis, ParserKind::Sccm, or duplicate CCM parser path.
This is a COMMENTED exact-head BLOCK review. PR #394 should remain draft/open. Native capture and real Windows acceptance remain explicitly pending under #319.
|
Implementation ownership claim — #319 privacy correction I own the exact-head P1 correction from fd405bd in isolated worktree /Users/Adam.Gell/repo/cmtraceopen/.worktrees/sccm-319-privacy-fd405-r2 on local branch codex/sccm-319-privacy-fd405-r2. No prior review or implementation worktree will be edited. Owned files: crates/cmtraceopen-parser/tests/sccm_client_intake.rs and crates/cmtraceopen-parser/src/sccm/client/intake.rs only. TDD order: commit focused test-only RED for identity/control-bearing provenance, then the smallest bounded validation helpers, focused GREEN, aggregate gates, non-force push after guarding remote fd405bd. CodeRabbit will be requested again on the corrected exact head. PR remains draft/open; native collection and real Windows acceptance stay pending. |
RED: cargo test --locked -p cmtraceopen-parser --test sccm_client_intake -- --nocapture ran 13 tests: 11 passed and 2 intended privacy regressions failed.
|
Correction response to the independent exact-head BLOCK review: #394 (review) New exact head: cc545fe The correction rejects identity/profile/domain-like arbitrary relative-path segments, absolute/traversal/separator encodings, NUL/control/non-ASCII and unsafe fingerprint characters before assessment projection. It preserves bounded namespaced fingerprints and approved collision-safe layouts: direct, current, lo, numbered, timestamped, root-a/root-b, evidence/sccm/client, and unknown-source forms. Verification at cc545fe:
Push was non-force after guarding remote/PR head fd405bd. PR remains draft/open; native #319 and real Windows acceptance remain pending. @coderabbitai review Please perform a fresh substantive review of exact head cc545fe, including the two-commit privacy correction. Focus on fail-closed public provenance, valid namespaced fingerprints, approved collision-safe path layouts, traversal/identity bypasses, pure Rust/wasm compatibility, and regression scope. A skipped or rate-limited response is not approval. The authenticated CodeRabbit CLI exact-range request was also attempted and returned a recoverable four-minute service rate limit before analysis; it will be retried. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
head='cc545fe8b8d5c56c19583556f92414df85a78fc2'
base='fd405bdf4cf527f20d3d778dedc0dc462d2f4229'
intake='crates/cmtraceopen-parser/src/sccm/client/intake.rs'
tests='crates/cmtraceopen-parser/tests/sccm_client_intake.rs'
printf '%s\n' '--- Exact commit and worktree state ---'
printf 'HEAD=%s\n' "$(git rev-parse HEAD)"
printf 'REQUESTED=%s\n' "$head"
git rev-parse "$head"
git merge-base --is-ancestor "$base" "$head"
git status --short
printf '%s\n' '--- Correction range ---'
git diff --name-status "$base" "$head"
git log --oneline --reverse "$base..$head"
git diff --check "$base" "$head"
printf '%s\n' '--- Changed-file outlines ---'
ast-grep outline "$intake" --items all --type function,struct,enum,impl
ast-grep outline "$tests" --items all --type function
printf '%s\n' '--- Privacy validation, projection, collision, and ordering locations ---'
rg -n -C 8 --glob '*.rs' \
'validate_bundle|validate_.*(path|fingerprint)|relative_path|path_fingerprint|fingerprint|Unsupported|physical_artifacts|expected_rotated_name|sort_by|collision|Invalid' \
"$intake" crates/cmtraceopen-parser/src/sccm
printf '%s\n' '--- Imports and platform-sensitive dependencies in intake module ---'
sed -n '1,120p' "$intake"
rg -n -C 2 --glob '*.rs' \
'std::fs|std::path|std::env|tokio|tauri|windows|wasm_bindgen|web_sys' \
"$intake" crates/cmtraceopen-parser/src/sccm/client
printf '%s\n' '--- Correction-range test changes ---'
git diff --minimal "$base" "$head" -- "$tests"
printf '%s\n' '--- Relevant public model contracts ---'
rg -n -C 6 --glob '*.rs' \
'SccmClient(Intake|Unsupported)|SccmArtifact|SccmRotation|SccmCoverageState|pathFingerprint|relativePath' \
crates/cmtraceopen-parser/src/sccm/models.rs crates/cmtraceopen-parser/src/sccmLength of output: 2515 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo='adamgell/cmtraceopen'
pr='394'
printf '%s\n' '--- Local checkout identity ---'
git rev-parse HEAD
git show -s --format='commit=%H%nparents=%P%nsubject=%s' HEAD
printf '%s\n' '--- GitHub pull request head and base ---'
gh pr view "$pr" --repo "$repo" \
--json number,state,isDraft,headRefName,headRefOid,baseRefName,baseRefOid,url
printf '%s\n' '--- GitHub pull request commits ---'
gh pr view "$pr" --repo "$repo" --json commits \
--jq '.commits[] | "\(.oid)\t\(.messageHeadline)"'
printf '%s\n' '--- Requested-prefix matches in remote PR commits ---'
gh pr view "$pr" --repo "$repo" --json commits \
--jq '.commits[].oid' | grep -i '^cc545fe8' || trueLength of output: 2079 🏁 Script executed: #!/bin/bash
set -euo pipefail
head='cc545fe86e28d184067db31269e7aa079d2d4a65'
base='fd405bdf4cf527f20d3d778dedc0dc462d2f4229'
intake='crates/cmtraceopen-parser/src/sccm/client/intake.rs'
tests='crates/cmtraceopen-parser/tests/sccm_client_intake.rs'
models='crates/cmtraceopen-parser/src/sccm/models.rs'
printf '%s\n' '--- Correction range and changed files ---'
git diff --name-status "$base" "$head"
git diff --check "$base" "$head"
git diff --minimal "$base" "$head" -- "$intake" "$tests"
printf '%s\n' '--- Intake module outline ---'
ast-grep outline "$intake" --items all --type function,struct,enum,impl
printf '%s\n' '--- Intake implementation, first half ---'
nl -ba "$intake" | sed -n '1,430p'
printf '%s\n' '--- Intake implementation, second half ---'
nl -ba "$intake" | sed -n '431,900p'
printf '%s\n' '--- Focused test definitions and provenance cases ---'
ast-grep outline "$tests" --items all --type function
rg -n -C 12 --glob '*.rs' \
'unsafe|provenance|relative.path|fingerprint|traversal|identity|profile|domain|control|non.ascii|collision|unknown|LocationServices' \
"$tests"
printf '%s\n' '--- Public models and serialization contracts ---'
rg -n -C 10 --glob '*.rs' \
'SccmRotation|SccmClient(Intake|Unsupported)|SccmArtifact|path_fingerprint|relative_path|impl Serialize' \
"$models"
printf '%s\n' '--- All construction and serialization sites for public intake output ---'
rg -n -C 5 --glob '*.rs' \
'\b(assess_client_intake|SccmClientIntakeBundle|SccmClientUnsupportedArtifact|to_value|to_string)\b' \
crates/cmtraceopen-parser/srcLength of output: 11298 🏁 Script executed: #!/bin/bash
set -euo pipefail
intake='crates/cmtraceopen-parser/src/sccm/client/intake.rs'
tests='crates/cmtraceopen-parser/tests/sccm_client_intake.rs'
models='crates/cmtraceopen-parser/src/sccm/models.rs'
printf '%s\n' '--- Intake module map ---'
ast-grep outline "$intake" --items all --type function,struct,enum
printf '%s\n' '--- Intake declarations, validation, and output construction ---'
nl -ba "$intake" | sed -n '1,260p'
nl -ba "$intake" | sed -n '260,540p'
nl -ba "$intake" | sed -n '540,780p'
printf '%s\n' '--- Public model declarations and serializers ---'
nl -ba "$models" | sed -n '240,460p'
printf '%s\n' '--- Exact references to provenance fields and output types ---'
rg -n -C 8 --glob '*.rs' \
'\.(relative_path|path_fingerprint)|relative_path:|path_fingerprint:|SccmClientIntakeArtifact|SccmClientUnsupportedArtifact' \
crates/cmtraceopen-parser/src crates/cmtraceopen-parser/tests
printf '%s\n' '--- Existing test helpers and full new test tail ---'
nl -ba "$tests" | sed -n '1,130p'
nl -ba "$tests" | sed -n '360,540p'
printf '%s\n' '--- Catalog group definitions and artifact-to-group matching ---'
rg -n -C 10 --glob '*.rs' \
'CLIENT_SOURCE|client-[a-z]|declared_client_source_groups|source_group|group.*artifact|artifact.*group' \
"$intake" crates/cmtraceopen-parser/src/sccmLength of output: 5046 🏁 Script executed: #!/bin/bash
set -euo pipefail
intake='crates/cmtraceopen-parser/src/sccm/client/intake.rs'
tests='crates/cmtraceopen-parser/tests/sccm_client_intake.rs'
models='crates/cmtraceopen-parser/src/sccm/models.rs'
numbered() {
awk '{ printf "%6d %s\n", NR, $0 }' "$1"
}
printf '%s\n' '--- Intake declarations and assessment pipeline ---'
numbered "$intake" | sed -n '1,360p'
printf '%s\n' '--- Intake validation, classification, output, and safety helpers ---'
numbered "$intake" | sed -n '360,740p'
printf '%s\n' '--- Public model declarations and serializers ---'
numbered "$models" | sed -n '240,460p'
printf '%s\n' '--- Provenance-field reads, writes, and serialization sites ---'
rg -n -C 7 --glob '*.rs' \
'\.(relative_path|path_fingerprint)|relative_path:|path_fingerprint:|SccmClientIntakeArtifact|SccmClientUnsupportedArtifact' \
crates/cmtraceopen-parser/src crates/cmtraceopen-parser/tests
printf '%s\n' '--- Test helper and privacy regression cases ---'
numbered "$tests" | sed -n '1,145p'
numbered "$tests" | sed -n '360,540p'
printf '%s\n' '--- Catalog definitions and artifact-to-group matching ---'
rg -n -C 10 --glob '*.rs' \
'CLIENT_SOURCE|client-[a-z]|declared_client_source_groups|matching_groups|accepted_basenames|logical_artifact_id' \
"$intake" crates/cmtraceopen-parser/src/sccmLength of output: 163173
The full SHA in the request does not exist on the PR. Its BLOCK — identity-bearing values still pass the relative-path allowlist.
The same problem exists for root segments. A domain-like value such as The new tests reject identity values only when they occupy an extra path segment or use forbidden characters. They do not test identity-like values in the accepted Use a bounded allowlist for client group segments. The client group values should be the exact 11 catalog identifiers plus I also confirmed that the correction range changes only the parser intake module and its tests. The intake module imports no filesystem, Tauri, Windows, or other native I/O API. I did not run builds or tests because this sandbox does not provide compiler execution. You are interacting with an AI system. |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
head='cc545fe86e28d184067db31269e7aa079d2d4a65'
base='fd405bdf4cf527f20d3d778dedc0dc462d2f4229'
printf '%s\n' '--- Exact head ---'
printf 'checked_out=%s\n' "$(git rev-parse HEAD)"
printf 'requested=%s\n' "$head"
git rev-parse "${head}^{commit}"
git merge-base --is-ancestor "$base" "$head"
printf 'base_is_ancestor=yes\n'
printf '%s\n' '--- Correction-range files ---'
git diff --name-status "$base" "$head"
git diff --stat "$base" "$head"
printf '%s\n' '--- Changed-file outlines ---'
for file in \
crates/cmtraceopen-parser/src/sccm/client/intake.rs \
crates/cmtraceopen-parser/tests/sccm_client_intake.rs
do
printf '\n### %s\n' "$file"
ast-grep outline "$file" --items all --type function,struct,enum,impl,trait 2>/dev/null || true
done
printf '%s\n' '--- Correction range ---'
git diff --find-renames --minimal "$base" "$head" -- \
crates/cmtraceopen-parser/src/sccm/client/intake.rs \
crates/cmtraceopen-parser/tests/sccm_client_intake.rs
printf '%s\n' '--- Related public contracts and exports ---'
rg -n -C 4 --glob '*.rs' \
'pub mod client|pub use .*intake|SccmClientIntake|SccmRotation|SccmClientIntakeError|PathFingerprint|path_fingerprint' \
crates/cmtraceopen-parser/src/sccmLength of output: 54422 🏁 Script executed: #!/bin/bash
set -euo pipefail
intake='crates/cmtraceopen-parser/src/sccm/client/intake.rs'
models='crates/cmtraceopen-parser/src/sccm/models.rs'
tests='crates/cmtraceopen-parser/tests/sccm_client_intake.rs'
printf '%s\n' '--- Intake imports, public types, source catalog, and assessment ---'
sed -n '1,357p' "$intake"
printf '%s\n' '--- Intake validation, classification, ordering, and all safety helpers ---'
sed -n '358,760p' "$intake"
printf '%s\n' '--- Rotation serialization and deserialization ---'
sed -n '260,410p' "$models"
printf '%s\n' '--- Focused intake tests ---'
sed -n '1,560p' "$tests"
printf '%s\n' '--- Intake module dependencies and prohibited API indicators ---'
rg -n -C 2 --glob '*.rs' \
'^(use|mod) |std::(fs|path|env|process|net)|tokio|tauri|reqwest|windows|wasm_bindgen|web_sys' \
"$intake" crates/cmtraceopen-parser/src/sccm/client/mod.rs
printf '%s\n' '--- Source group declarations and group-path references ---'
rg -n -C 5 --glob '*.rs' \
'CLIENT_SOURCE_GROUPS|ClientSourceGroupSpec|logical_artifact_id|client-policy-agent|client-app-enforce|evidence/sccm/client' \
crates/cmtraceopen-parser/src/sccm/client "$tests"Length of output: 87655 🏁 Script executed: #!/bin/bash
set -euo pipefail
intake='crates/cmtraceopen-parser/src/sccm/client/intake.rs'
printf '%s\n' '--- Line-numbered public projection and validation ---'
nl -ba "$intake" | sed -n '280,445p'
nl -ba "$intake" | sed -n '475,690p'
printf '%s\n' '--- All relative-path and group/root validation references ---'
rg -n -C 3 --glob '*.rs' \
'\b(relative_path|is_safe_relative_path|is_safe_client_bundle_group|is_safe_root_path_segment|root-[A-Za-z0-9-]+|client-[A-Za-z0-9-]+)\b' \
crates/cmtraceopen-parser/src/sccm/client crates/cmtraceopen-parser/tests/sccm_client_intake.rs
printf '%s\n' '--- Fixture path layouts ---'
fd 'manifest.json' crates/cmtraceopen-parser/tests/fixtures/sccm/client/intake \
-x sh -c 'echo "--- $1"; rg -n "\"relativePath\"|\"pathFingerprint\"" "$1"' sh {}Length of output: 1196 🏁 Script executed: #!/bin/bash
set -euo pipefail
intake='crates/cmtraceopen-parser/src/sccm/client/intake.rs'
printf '%s\n' '--- Line-numbered public projection and validation ---'
awk 'NR >= 280 && NR <= 445 { printf "%6d\t%s\n", NR, $0 }' "$intake"
awk 'NR >= 475 && NR <= 690 { printf "%6d\t%s\n", NR, $0 }' "$intake"
printf '%s\n' '--- All relative-path and group/root validation references ---'
rg -n -C 3 --glob '*.rs' \
'\b(relative_path|is_safe_relative_path|is_safe_client_bundle_group|is_safe_root_path_segment)\b' \
crates/cmtraceopen-parser/src/sccm/client crates/cmtraceopen-parser/tests/sccm_client_intake.rs
printf '%s\n' '--- Fixture path layouts ---'
fd 'manifest.json' crates/cmtraceopen-parser/tests/fixtures/sccm/client/intake \
-x sh -c 'echo "--- $1"; rg -n "\"relativePath\"|\"pathFingerprint\"" "$1"' sh {}Length of output: 56573
I found one blocking issue.
I also confirmed the following by inspection:
I did not run builds or tests. This sandbox permits only read-only static inspection. ✅ Action performedReview finished.
|
REVIEW RED: focused client intake ran 13 tests: 12 passed and the identity-slot bypass regression failed.
|
@coderabbitai review Your exact-head blocker at #394 (comment) was valid and is corrected at exact head 06f8693. Review RED: c9f2e6c — focused intake 12/13, with the accepted-slot identity bypass failing. The group segment is now allowlisted from the bounded CLIENT_SOURCE_GROUPS table plus client-location-services-shared and unknown. Collision roots now accept only the committed synthetic root-a/root-b fixtures or opaque 16/64-character lowercase hexadecimal handles. Permanent regressions reject client-RealUser, client-corp-example-test, root-realuser, and root-corp-example-test while preserving shared LocationServices and opaque root-0123456789abcdef paths. Fresh verification at 06f8693: focused 13/13; spine 136/136; full parser 742; strict Clippy; Rust 1.88 wasm32; TypeScript; 12 JSON; scoped rustfmt/diff/clean all pass. Global fmt remains only the inherited 17-file non-#319 baseline. Please perform a fresh substantive review of exact head 06f8693 and explicitly report any remaining actionable finding. A skipped or rate-limited response is not approval. PR stays draft/open; native and real Windows acceptance remain pending. |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
head='06f8693005c2439279133936a7023a03bf9ba514'
prior='cc545fe86e28d184067db31269e7aa079d2d4a65'
intake='crates/cmtraceopen-parser/src/sccm/client/intake.rs'
tests='crates/cmtraceopen-parser/tests/sccm_client_intake.rs'
models='crates/cmtraceopen-parser/src/sccm/models.rs'
printf '%s\n' '--- Exact PR and commit identity ---'
printf 'checked_out=%s\n' "$(git rev-parse HEAD)"
git rev-parse "${head}^{commit}"
git merge-base --is-ancestor "$prior" "$head" && echo 'prior_is_ancestor=yes'
gh pr view 394 --repo adamgell/cmtraceopen \
--json state,isDraft,headRefOid,baseRefOid,url \
--jq '"pr=\(.url) state=\(.state) draft=\(.isDraft) head=\(.headRefOid) base=\(.baseRefOid)"'
printf '%s\n' '--- Correction range ---'
git log --oneline --reverse "${prior}..${head}"
git diff --name-status "$prior" "$head"
git diff --check "$prior" "$head"
git diff --find-renames --minimal "$prior" "$head" -- "$intake" "$tests"
printf '%s\n' '--- Changed-file maps ---'
ast-grep outline "$intake" --items all --type function,struct,enum,impl
ast-grep outline "$tests" --items all --type function
printf '%s\n' '--- Validation, classification, projection, collision, and ordering ---'
nl -ba "$intake" | sed -n '1,780p'
printf '%s\n' '--- Public model contracts and serializers ---'
nl -ba "$models" | sed -n '240,470p'
printf '%s\n' '--- Focused privacy, path, collision, and ordering tests ---'
rg -n -C 16 --glob '*.rs' \
'privacy|identity|profile|domain|relative.path|fingerprint|root-|client-|LocationServices|collision|ordering|rotation|unsupported|InvalidRelativePath' \
"$tests"
printf '%s\n' '--- Intake call sites and platform-sensitive dependencies ---'
rg -n -C 5 --glob '*.rs' \
'\b(assess_client_intake|SccmClientIntakeBundle|SccmClientUnsupportedArtifact)\b' \
crates/cmtraceopen-parser/src crates/cmtraceopen-parser/tests
rg -n -C 2 --glob '*.rs' \
'std::(fs|path|env|process|net)|tokio|tauri|reqwest|windows|wasm_bindgen|web_sys' \
"$intake" crates/cmtraceopen-parser/src/sccm/client/mod.rs || trueLength of output: 6046 🏁 Script executed: #!/bin/bash
set -euo pipefail
head='06f8693005c2439279133936a7023a03bf9ba514'
intake='crates/cmtraceopen-parser/src/sccm/client/intake.rs'
tests='crates/cmtraceopen-parser/tests/sccm_client_intake.rs'
models='crates/cmtraceopen-parser/src/sccm/models.rs'
test "$(git rev-parse HEAD)" = "$head"
printf '%s\n' '--- Intake and test maps ---'
ast-grep outline "$intake" --items all --type function,struct,enum
ast-grep outline "$tests" --items all --type function
printf '%s\n' '--- Intake catalog and assessment pipeline ---'
nl -ba "$intake" | sed -n '1,360p'
printf '%s\n' '--- Intake validation, classification, projection, ordering, and safety helpers ---'
nl -ba "$intake" | sed -n '360,780p'
printf '%s\n' '--- Public models and serializers used by intake ---'
nl -ba "$models" | sed -n '240,470p'
printf '%s\n' '--- Focused test helpers and all privacy-related test bodies ---'
nl -ba "$tests" | sed -n '1,160p'
nl -ba "$tests" | sed -n '340,550p'
printf '%s\n' '--- All provenance field reads and writes ---'
rg -n -C 7 --glob '*.rs' \
'\.(relative_path|path_fingerprint|original_path|host)|relative_path:|path_fingerprint:|original_path:|host:' \
crates/cmtraceopen-parser/src/sccm crates/cmtraceopen-parser/tests/sccm_client_intake.rs
printf '%s\n' '--- Intake API use and platform-sensitive dependencies ---'
rg -n -C 5 --glob '*.rs' \
'\b(assess_client_intake|SccmClientIntakeBundle|SccmClientUnsupportedArtifact)\b' \
crates/cmtraceopen-parser/src crates/cmtraceopen-parser/tests
rg -n -C 2 --glob '*.rs' \
'std::(fs|path|env|process|net)|tokio|tauri|reqwest|windows|wasm_bindgen|web_sys' \
"$intake" crates/cmtraceopen-parser/src/sccm/client/mod.rs || trueLength of output: 6853 🏁 Script executed: #!/bin/bash
set -euo pipefail
head='06f8693005c2439279133936a7023a03bf9ba514'
intake='crates/cmtraceopen-parser/src/sccm/client/intake.rs'
tests='crates/cmtraceopen-parser/tests/sccm_client_intake.rs'
models='crates/cmtraceopen-parser/src/sccm/models.rs'
test "$(git rev-parse HEAD)" = "$head"
show_lines() {
awk -v start="$2" -v end="$3" \
'NR >= start && NR <= end { printf "%6d\t%s\n", NR, $0 }' "$1"
}
printf '%s\n' '--- Intake catalog and assessment pipeline ---'
show_lines "$intake" 1 357
printf '%s\n' '--- Intake validation, matching, projection, ordering, and path safety ---'
show_lines "$intake" 358 740
printf '%s\n' '--- Public model contracts and serializers ---'
show_lines "$models" 240 470
printf '%s\n' '--- Test helpers and privacy/path regressions ---'
show_lines "$tests" 1 160
show_lines "$tests" 240 550
printf '%s\n' '--- All public provenance reads and writes ---'
rg -n -C 7 --glob '*.rs' \
'\.(relative_path|path_fingerprint|original_path|host)|relative_path:|path_fingerprint:|original_path:|host:' \
crates/cmtraceopen-parser/src/sccm crates/cmtraceopen-parser/tests/sccm_client_intake.rs
printf '%s\n' '--- Intake API use and platform-sensitive dependencies ---'
rg -n -C 5 --glob '*.rs' \
'\b(assess_client_intake|SccmClientIntakeBundle|SccmClientUnsupportedArtifact)\b' \
crates/cmtraceopen-parser/src crates/cmtraceopen-parser/tests
rg -n -C 2 --glob '*.rs' \
'std::(fs|path|env|process|net)|tokio|tauri|reqwest|windows|wasm_bindgen|web_sys' \
"$intake" crates/cmtraceopen-parser/src/sccm/client/mod.rs || trueLength of output: 131924
I found one remaining actionable issue.
I confirmed the prior relative-path finding is fixed.
I did not run builds or tests. This review environment supports read-only static inspection only. ✅ Action performedReview finished.
|
CODERABBIT RED: focused client intake ran 13 tests: 12 passed and the impossible timestamp regression failed.
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/cmtraceopen-parser/tests/sccm_client_intake.rs`:
- Around line 394-411: Extend
identity_bearing_relative_paths_fail_before_public_projection with cases where
client-group and root-label namespace segments contain arbitrary identity
values, including client-realuser/current/PolicyAgent.log,
client-app-enforce/root-realuser/current/AppEnforce.log, and the shared
LocationServices.log path. Replace prefix-based acceptance with bounded
allowlists for client group identifiers and root labels, apply the same
validation to LocationServices.log, and assert InvalidRelativePath for every
rejected path.
🪄 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: 93694b73-d731-444a-a1ef-cf6f07e4d0d5
📒 Files selected for processing (4)
crates/cmtraceopen-parser/src/sccm/client/intake.rscrates/cmtraceopen-parser/src/sccm/client/mod.rscrates/cmtraceopen-parser/src/sccm/mod.rscrates/cmtraceopen-parser/tests/sccm_client_intake.rs
|
@coderabbitai review Please perform a substantive exact-head review of |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo='adamgell/cmtraceopen'
pr='394'
head='54470563724af20de87aeca5e5a50e4090de5091'
base='fd405bdf4cf527f20d3d778dedc0dc462d2f4229'
intake='crates/cmtraceopen-parser/src/sccm/client/intake.rs'
tests='crates/cmtraceopen-parser/tests/sccm_client_intake.rs'
models='crates/cmtraceopen-parser/src/sccm/models.rs'
printf '%s\n' '--- Commit and PR identity ---'
printf 'checked_out=%s\n' "$(git rev-parse HEAD)"
printf 'requested=%s\n' "$head"
git rev-parse "${head}^{commit}"
git merge-base --is-ancestor "$base" "$head" && echo 'base_is_ancestor=yes'
gh pr view "$pr" --repo "$repo" \
--json state,isDraft,headRefOid,baseRefOid,url \
--jq '"pr=\(.url) state=\(.state) draft=\(.isDraft) head=\(.headRefOid) base=\(.baseRefOid)"'
printf '%s\n' '--- Review range ---'
git log --oneline --reverse "${base}..${head}"
git diff --name-status "$base" "$head"
git diff --check "$base" "$head"
git diff --find-renames --minimal "$base" "$head" -- "$intake" "$tests"
printf '%s\n' '--- Relevant file outlines ---'
ast-grep outline "$intake" --items all --type function,struct,enum,impl
ast-grep outline "$tests" --items all --type function
ast-grep outline "$models" --items all --type function,struct,enum,impl
printf '%s\n' '--- Intake implementation: public types, validation, projection, and ordering ---'
nl -ba "$intake" | sed -n '1,800p'
printf '%s\n' '--- Rotation model serialization contract ---'
nl -ba "$models" | sed -n '240,470p'
printf '%s\n' '--- Focused provenance, fingerprint, path, rotation, collision, and ordering tests ---'
rg -n -C 14 --glob '*.rs' \
'provenance|privacy|identity|fingerprint|relative.path|Invalid(PathFingerprint|RelativePath|Rotation)|Timestamped|timestamp|collision|ordering|LocationServices|unsupported' \
"$tests"
printf '%s\n' '--- All provenance reads and writes in production code ---'
rg -n -C 7 --glob '*.rs' \
'\.(relative_path|path_fingerprint|original_path|host)|relative_path:|path_fingerprint:|original_path:|host:' \
crates/cmtraceopen-parser/src/sccm
printf '%s\n' '--- Platform-sensitive dependencies in intake module ---'
rg -n -C 2 --glob '*.rs' \
'std::(fs|path|env|process|net)|tokio|tauri|reqwest|windows|wasm_bindgen|web_sys' \
"$intake" crates/cmtraceopen-parser/src/sccm/client/mod.rs || trueLength of output: 13520 🏁 Script executed: #!/bin/bash
set -euo pipefail
head='54470563724af20de87aeca5e5a50e4090de5091'
intake='crates/cmtraceopen-parser/src/sccm/client/intake.rs'
tests='crates/cmtraceopen-parser/tests/sccm_client_intake.rs'
models='crates/cmtraceopen-parser/src/sccm/models.rs'
test "$(git rev-parse HEAD)" = "$head"
printf '%s\n' '--- Intake map ---'
ast-grep outline "$intake" --items all --type function,struct,enum
printf '%s\n' '--- Focused-test map ---'
ast-grep outline "$tests" --items all --type function
printf '%s\n' '--- Validation and public-projection pipeline ---'
nl -ba "$intake" | sed -n '250,760p'
printf '%s\n' '--- Full path and fingerprint validation context ---'
rg -n -C 18 --glob '*.rs' \
'fn validate_bundle|is_safe_path_identity|is_safe_relative_path|is_safe_path_segment|path_fingerprint|relative_path|InvalidPathFingerprint|InvalidRelativePath' \
"$intake"
printf '%s\n' '--- Classification, collision checks, output construction, and deterministic ordering ---'
rg -n -C 16 --glob '*.rs' \
'matching_groups|collision|sort_by|cmp\(|intake_fragment|unsupported|physical_artifacts|fragments|expected_rotated_name' \
"$intake"
printf '%s\n' '--- Rotation serialization and canonical-timestamp implementation ---'
rg -n -C 15 --glob '*.rs' \
'SccmRotation|impl Serialize|serialize|is_canonical_rotation_timestamp|Timestamped' \
"$models" crates/cmtraceopen-parser/src/sccm
printf '%s\n' '--- Full focused privacy and timestamp test bodies ---'
nl -ba "$tests" | sed -n '250,560p'
printf '%s\n' '--- Path-fingerprint fixture values and all production construction sites ---'
rg -n -C 5 --glob '*.rs' \
'path_fingerprint:|path_fingerprint\s*=|pathFingerprint|synthetic:|sha256:' \
crates/cmtraceopen-parser/src crates/cmtraceopen-parser/tests
printf '%s\n' '--- Native or platform-dependent intake dependencies ---'
rg -n -C 2 --glob '*.rs' \
'std::(fs|path|env|process|net)|tokio|tauri|reqwest|windows|wasm_bindgen|web_sys' \
"$intake" crates/cmtraceopen-parser/src/sccm/client/mod.rs || trueLength of output: 7148 🏁 Script executed: #!/bin/bash
set -euo pipefail
head='54470563724af20de87aeca5e5a50e4090de5091'
intake='crates/cmtraceopen-parser/src/sccm/client/intake.rs'
tests='crates/cmtraceopen-parser/tests/sccm_client_intake.rs'
models='crates/cmtraceopen-parser/src/sccm/models.rs'
test "$(git rev-parse HEAD)" = "$head"
printf '%s\n' '--- Validation, classification, projection, collision, and ordering ---'
sed -n '281,710p' "$intake" | awk '{ printf "%6d %s\n", NR + 280, $0 }'
printf '%s\n' '--- Focused regression bodies ---'
sed -n '285,525p' "$tests" | awk '{ printf "%6d %s\n", NR + 284, $0 }'
printf '%s\n' '--- Rotation serialization and timestamp validator ---'
rg -n -C 18 --glob '*.rs' \
'enum SccmRotation|impl Serialize for SccmRotation|fn is_canonical_rotation_timestamp|is_canonical_rotation_timestamp|Timestamped' \
"$models" crates/cmtraceopen-parser/src/sccm/rotation.rs crates/cmtraceopen-parser/src/sccm
printf '%s\n' '--- Every path-fingerprint validator call and public-field projection ---'
rg -n -C 10 --glob '*.rs' \
'is_safe_path_identity\(|InvalidPathFingerprint|path_fingerprint: source\.path_fingerprint|path_fingerprint:.*path_fingerprint' \
"$intake"
printf '%s\n' '--- All committed fingerprint literals in fixtures and focused tests ---'
rg -n -C 2 --glob '*.json' --glob '*.rs' \
'"pathFingerprint"|path_fingerprint\s*=|path_fingerprint:|synthetic:|sha256:' \
crates/cmtraceopen-parser/tests/fixtures/sccm/client/intake "$tests"
printf '%s\n' '--- Intake import list and prohibited dependencies ---'
sed -n '1,24p' "$intake"
rg -n -C 2 --glob '*.rs' \
'std::(fs|path|env|process|net)|tokio|tauri|reqwest|windows|wasm_bindgen|web_sys' \
"$intake" crates/cmtraceopen-parser/src/sccm/client/mod.rs || trueLength of output: 112087
I found one actionable blocking issue.
I also confirmed the following:
I did not run builds or tests. This environment supports read-only static inspection only. ✅ Action performedReview finished.
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
crates/cmtraceopen-parser/src/sccm/client/intake.rs (2)
1455-1459: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider rejecting an empty value in
is_lowercase_hex.
is_lowercase_hex("")returnstrue, becauseallon an empty iterator istrue. Both current callers gate on an exact length first, so no defect exists today. A future caller that omits the length gate would silently accept an empty handle. An explicit emptiness guard removes that trap.🛡️ Proposed hardening
fn is_lowercase_hex(value: &str) -> bool { - value + !value.is_empty() + && value .bytes() .all(|byte| byte.is_ascii_digit() || matches!(byte, b'a'..=b'f')) }🤖 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 1455 - 1459, Update is_lowercase_hex to return false when value is empty, while preserving the existing lowercase hexadecimal validation for non-empty values.
243-443: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider guarding the wire mirrors against field drift.
Each public projection type has a hand-written wire twin plus two
Fromimpls. If a field is added toSccmClientIntakeFragmentand not toSccmClientIntakeFragmentWire, the field disappears from the serialized assessment without any compile error. TheFrom<SccmClientIntakeFragmentWire>direction fails to compile, but theFrom<&SccmClientIntakeFragment>direction does not.A destructuring pattern in the
From<&T>impls makes the omission a compile error.♻️ Example for one impl
impl From<&SccmClientIntakeFragment> for SccmClientIntakeFragmentWire { fn from(fragment: &SccmClientIntakeFragment) -> Self { + let SccmClientIntakeFragment { + artifact_id, + basename, + rotation, + coverage, + path_fingerprint, + rotation_lineage, + relative_path, + fragment_complete, + configmgr_version, + collected_at_utc, + encoding, + } = fragment; Self { - artifact_id: fragment.artifact_id.clone(), + artifact_id: artifact_id.clone(),🤖 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 243 - 443, Update the `From<&SccmClientIntakeFragment>` and equivalent `From<&...>` wire-conversion implementations to destructure their source public types before constructing the wire mirrors, so adding a source field without updating the wire type causes a compile error. Apply this to `SccmClientIntakeGroup`, `SccmClientIntakeCoverageGap`, `SccmClientUnsupportedArtifact`, and `SccmClientIntakeAssessment`, preserving the existing field mappings.crates/cmtraceopen-parser/tests/fixtures/sccm/client/intake/collision/expected.json (1)
174-188: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe unique-contents claim is not pinned for this fixture pair.
The fixture README states that the two
AppEnforce.logfiles keep unique contents. Both provenance entries here declarebytesCopied: 180and neither declares asha256. The fixture test only compares file length whensha256is absent, so two byte-identical evidence files would still pass. Adding asha256to both entries pins the uniqueness that the README claims.This is optional. The distinct
pathFingerprintandrelativePathvalues already prove the collision behavior that the scenario targets.🤖 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/tests/fixtures/sccm/client/intake/collision/expected.json` around lines 174 - 188, Add the correct sha256 value to both artifactProvenance entries for fixture-collision-app-enforce-root-a-current and fixture-collision-app-enforce-root-b-current in nativeDesignPending, preserving their existing metadata and ensuring the hashes reflect their distinct file contents.crates/cmtraceopen-parser/src/sccm/mod.rs (1)
2-2: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider whether the double public path is intended.
Line 2 makes
clientpublic and line 13 glob re-exports it. Every intake item therefore resolves at bothsccm::client::Xandsccm::X. The PR objectives describe the API assccm::client::intake, which suggests the module path is the intended one.A glob re-export also couples this line to the sibling
servermodule: any future name shared byclient::*andserver::*becomes an ambiguity error here. Keepingpub mod client;without the glob, or re-exporting named items, avoids that coupling.Also applies to: 13-13
🤖 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/mod.rs` at line 2, Remove the glob re-export of client items from the SCCM module, keeping client publicly accessible through sccm::client and preserving the intended sccm::client::intake API. Update the module declarations around pub mod client and the corresponding re-export to avoid collisions with server names.crates/cmtraceopen-parser/src/sccm/findings.rs (1)
2342-2355: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe second normalization of each component is redundant.
Line 2338 splits the stem on every character that is not ASCII alphanumeric and not
_. Each resulting component therefore already contains only characters thatnormalize_catalog_identityretains. The call at line 2340 only lowercases. Replacing it with a direct lowercase makes the intent explicit and avoids implying that further stripping can occur.This is a readability point only. The current behavior is correct.
🤖 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/findings.rs` around lines 2342 - 2355, In the basename component construction immediately before the ranges extension, replace the repeated normalize_catalog_identity call with direct lowercasing of each already-sanitized component. Preserve the existing component splitting and matching behavior; this is only a readability change.crates/cmtraceopen-parser/tests/sccm_spine_contract.rs (1)
3936-3972: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueMatch the deserializer error variant exactly. Replace the
Debugsubstring check with a structured or exact variant comparison. A future variant namedInvalidArtifactRequestReasonLengthcould satisfy the currentcontainscheck.🤖 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/tests/sccm_spine_contract.rs` around lines 3936 - 3972, Update the deserializer assertion in the cases loop to compare the returned error against the expected error variant structurally or exactly, rather than matching a Debug-format substring. Preserve the existing validation checks and ensure similarly named variants such as InvalidArtifactRequestReasonLength cannot be accepted accidentally.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/cmtraceopen-parser/tests/fixtures/sccm/client/README.md`:
- Around line 48-53: Clarify the noncapture-artifact rule in the fixture README
by explicitly stating that “bytesCopied: 0” and a null relative path apply to
manifest.json; note that such artifacts are omitted from expected.json’s
nativeDesignPending.artifactProvenance.
In `@crates/cmtraceopen-parser/tests/sccm_client_intake.rs`:
- Around line 1302-1307: Update the assertion message in the client-content
group coverage test to describe the actual severity ordering: Capped outranks
Absent, which outranks Captured. Leave the assertions and coverage
implementation unchanged.
---
Nitpick comments:
In `@crates/cmtraceopen-parser/src/sccm/client/intake.rs`:
- Around line 1455-1459: Update is_lowercase_hex to return false when value is
empty, while preserving the existing lowercase hexadecimal validation for
non-empty values.
- Around line 243-443: Update the `From<&SccmClientIntakeFragment>` and
equivalent `From<&...>` wire-conversion implementations to destructure their
source public types before constructing the wire mirrors, so adding a source
field without updating the wire type causes a compile error. Apply this to
`SccmClientIntakeGroup`, `SccmClientIntakeCoverageGap`,
`SccmClientUnsupportedArtifact`, and `SccmClientIntakeAssessment`, preserving
the existing field mappings.
In `@crates/cmtraceopen-parser/src/sccm/findings.rs`:
- Around line 2342-2355: In the basename component construction immediately
before the ranges extension, replace the repeated normalize_catalog_identity
call with direct lowercasing of each already-sanitized component. Preserve the
existing component splitting and matching behavior; this is only a readability
change.
In `@crates/cmtraceopen-parser/src/sccm/mod.rs`:
- Line 2: Remove the glob re-export of client items from the SCCM module,
keeping client publicly accessible through sccm::client and preserving the
intended sccm::client::intake API. Update the module declarations around pub mod
client and the corresponding re-export to avoid collisions with server names.
In
`@crates/cmtraceopen-parser/tests/fixtures/sccm/client/intake/collision/expected.json`:
- Around line 174-188: Add the correct sha256 value to both artifactProvenance
entries for fixture-collision-app-enforce-root-a-current and
fixture-collision-app-enforce-root-b-current in nativeDesignPending, preserving
their existing metadata and ensuring the hashes reflect their distinct file
contents.
In `@crates/cmtraceopen-parser/tests/sccm_spine_contract.rs`:
- Around line 3936-3972: Update the deserializer assertion in the cases loop to
compare the returned error against the expected error variant structurally or
exactly, rather than matching a Debug-format substring. Preserve the existing
validation checks and ensure similarly named variants such as
InvalidArtifactRequestReasonLength cannot be accepted accidentally.
🪄 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: 73c57035-cf09-46b6-8555-916dbb4480fb
📒 Files selected for processing (16)
crates/cmtraceopen-parser/src/sccm/catalog.rscrates/cmtraceopen-parser/src/sccm/client/intake.rscrates/cmtraceopen-parser/src/sccm/client/mod.rscrates/cmtraceopen-parser/src/sccm/findings.rscrates/cmtraceopen-parser/src/sccm/mod.rscrates/cmtraceopen-parser/tests/fixtures/sccm/client/README.mdcrates/cmtraceopen-parser/tests/fixtures/sccm/client/intake/access-denied/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/client/intake/capped/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/client/intake/collision/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/client/intake/complete/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/client/intake/missing-root/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/client/intake/rotations/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/client/intake/rotations/manifest.jsoncrates/cmtraceopen-parser/tests/sccm_client_intake.rscrates/cmtraceopen-parser/tests/sccm_spine_contract.rsdocs/sccm/preparation/issue-319-client-intake.md
CodeRabbit feedback addressedValidated both exact-head comments independently and corrected the two wording defects in commit 05d4bf0:
Verification: focused client-intake suite 47/47, scoped Rust 1.88 rustfmt, and git diff --check passed. Global cargo fmt --check --all still reports the pre-existing unrelated formatting baseline; neither edited file contributes to it. A fresh exact-head CodeRabbit cycle is now required before this draft can move forward. |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated no new comments.
Suppressed comments (1)
crates/cmtraceopen-parser/src/sccm/client/intake.rs:333
SccmClientIntakeCoverageGapWire.artifact_idis configured withskip_serializing_if = "Option::is_none", soartifactIdwill be omitted (not serialized asnull) for group-level gaps where there is no specific artifact. The fixture oracles currently include"artifactId": nullfor these entries, and other optional gap/fragment fields are serialized as explicitnull, so omitting this one makes the public JSON shape inconsistent and harder for downstream consumers to rely on.
Consider removing skip_serializing_if (keep default) so the field is always present and round-trips cleanly as null when absent.
struct SccmClientIntakeCoverageGapWire {
logical_artifact_id: String,
#[serde(default, skip_serializing_if = "Option::is_none")]
artifact_id: Option<String>,
role: SccmRole,
coverage: SccmCoverageState,
reason: String,
}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
crates/cmtraceopen-parser/tests/sccm_spine_contract.rs (2)
3887-3933: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a rejection case for a single component of the multi-dot basename.
The new range logic in
crates/cmtraceopen-parser/src/sccm/findings.rssplitsclient.msiinto the componentsclientandmsi, then authorizes the token window for each. The five cases here cover an unknown multi-dot basename, a mismatched logical ID, a mismatched role, a glob, and unbounded language. No case asserts that a bare component is still rejected.A case such as
clientMsiwith the reasonCollect the complete client.log file.pins the boundary that the component split creates.💚 Proposed additional case
( "unknown multi-dot basename", finding_request( "clientMsi", SccmRole::Client, "Collect the complete client.unknown.log file.", ), SccmFindingValidationError::InvalidArtifactRequestReason, ), + ( + "bare component of the multi-dot basename", + finding_request( + "clientMsi", + SccmRole::Client, + "Collect the complete client.log file.", + ), + SccmFindingValidationError::InvalidArtifactRequestReason, + ),🤖 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/tests/sccm_spine_contract.rs` around lines 3887 - 3933, Add a rejection case to the cases table for finding_request("clientMsi", SccmRole::Client, "Collect the complete client.log file.") and expect SccmFindingValidationError::InvalidArtifactRequestReason, covering rejection of a bare component from the multi-dot basename.
3959-3971: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider asserting the deserializer error by value.
The builder check at Line 3946 and the direct validation check at Line 3952 compare
expected_errorby value. The deserializer check compares a substring of the error text against theDebugspelling of the variant. That assertion is weaker. It passes if the serde error text happens to contain the variant name for another reason, and it breaks if the error text format changes.The current form still fails when deserialization succeeds, so there is a safe fallback. Treat this as a test-precision improvement.
🤖 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/tests/sccm_spine_contract.rs` around lines 3959 - 3971, Update the deserializer assertion in the SccmFinding test to compare the returned error against expected_error by value, matching the builder and direct validation checks. Keep successful deserialization as a failure, and avoid relying on formatted error text or Debug-string substring matching.crates/cmtraceopen-parser/src/sccm/catalog.rs (1)
632-634: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider moving CCM-record eligibility onto
CatalogSpec.
catalog_entry_uses_ccm_recordsdecides eligibility by matchinglogical_nameagainst a string literal list. The fact then lives away from the catalog entry that owns it. A future rename ofclientMsiorreportingEventssilently re-enables raw CCM handling for that entry.The spine contract test currently pins the expected tuples, so a rename would fail the suite. This is therefore a readability and locality improvement, not a defect.
An alternative is a
uses_ccm_records: boolfield onCatalogSpec, set per entry.🤖 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/catalog.rs` around lines 632 - 634, Move CCM-record eligibility into CatalogSpec by adding an appropriate method or field that defines whether each catalog entry uses CCM records, initialized for clientMsi and reportingEvents as ineligible. Update catalog_entry_uses_ccm_records to delegate to that CatalogSpec-owned definition instead of matching logical_name string literals, while preserving the existing tuple expectations and 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 `@crates/cmtraceopen-parser/src/sccm/catalog.rs`:
- Around line 632-634: Move CCM-record eligibility into CatalogSpec by adding an
appropriate method or field that defines whether each catalog entry uses CCM
records, initialized for clientMsi and reportingEvents as ineligible. Update
catalog_entry_uses_ccm_records to delegate to that CatalogSpec-owned definition
instead of matching logical_name string literals, while preserving the existing
tuple expectations and behavior.
In `@crates/cmtraceopen-parser/tests/sccm_spine_contract.rs`:
- Around line 3887-3933: Add a rejection case to the cases table for
finding_request("clientMsi", SccmRole::Client, "Collect the complete client.log
file.") and expect SccmFindingValidationError::InvalidArtifactRequestReason,
covering rejection of a bare component from the multi-dot basename.
- Around line 3959-3971: Update the deserializer assertion in the SccmFinding
test to compare the returned error against expected_error by value, matching the
builder and direct validation checks. Keep successful deserialization as a
failure, and avoid relying on formatted error text or Debug-string substring
matching.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4995bdad-71c3-44a5-a8fd-121492122b6f
📒 Files selected for processing (16)
crates/cmtraceopen-parser/src/sccm/catalog.rscrates/cmtraceopen-parser/src/sccm/client/intake.rscrates/cmtraceopen-parser/src/sccm/client/mod.rscrates/cmtraceopen-parser/src/sccm/findings.rscrates/cmtraceopen-parser/src/sccm/mod.rscrates/cmtraceopen-parser/tests/fixtures/sccm/client/README.mdcrates/cmtraceopen-parser/tests/fixtures/sccm/client/intake/access-denied/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/client/intake/capped/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/client/intake/collision/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/client/intake/complete/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/client/intake/missing-root/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/client/intake/rotations/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/client/intake/rotations/manifest.jsoncrates/cmtraceopen-parser/tests/sccm_client_intake.rscrates/cmtraceopen-parser/tests/sccm_spine_contract.rsdocs/sccm/preparation/issue-319-client-intake.md
|
@coderabbitai full review |
Exact-head review adjudicationCurrent head: Validated the latest CodeRabbit nitpicks and Copilot suppressed observation against the contracts:
Verification at this exact head:
Fresh exact-head CodeRabbit, Copilot, and hosted CI cycles are required. The PR remains draft. |
✅ 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.
🧹 Nitpick comments (1)
crates/cmtraceopen-parser/src/sccm/catalog.rs (1)
632-634: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider moving CCM-record eligibility onto
CatalogSpec.
catalog_entry_uses_ccm_recordsmatches onlogical_namestring literals. A rename ofclientMsiorreportingEventscompiles cleanly and silently flips the entry back totrue, which routes a non-CCM supplement through raw CCM parsing. Auses_ccm_records: boolfield onCatalogSpeckeeps the eligibility next to the entry it describes and removes the string coupling.♻️ Sketch of the field-based alternative
struct CatalogSpec { basename: &'static str, logical_name: &'static str, role: SccmRole, family: SccmArtifactFamily, + uses_ccm_records: bool, }-fn catalog_entry_uses_ccm_records(entry: &CatalogSpec) -> bool { - !matches!(entry.logical_name, "clientMsi" | "reportingEvents") -} +fn catalog_entry_uses_ccm_records(entry: &CatalogSpec) -> bool { + entry.uses_ccm_records +}🤖 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/catalog.rs` around lines 632 - 634, Move CCM-record eligibility onto CatalogSpec by adding a uses_ccm_records boolean field to each catalog entry and updating catalog_entry_uses_ccm_records to return that field. Initialize the field explicitly for clientMsi and reportingEvents as false and for other applicable entries as true, then remove the logical_name string matching.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/cmtraceopen-parser/src/sccm/catalog.rs`:
- Around line 632-634: Move CCM-record eligibility onto CatalogSpec by adding a
uses_ccm_records boolean field to each catalog entry and updating
catalog_entry_uses_ccm_records to return that field. Initialize the field
explicitly for clientMsi and reportingEvents as false and for other applicable
entries as true, then remove the logical_name string matching.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2c46eb38-7edf-4449-9811-64226cd3826a
📒 Files selected for processing (16)
crates/cmtraceopen-parser/src/sccm/catalog.rscrates/cmtraceopen-parser/src/sccm/client/intake.rscrates/cmtraceopen-parser/src/sccm/client/mod.rscrates/cmtraceopen-parser/src/sccm/findings.rscrates/cmtraceopen-parser/src/sccm/mod.rscrates/cmtraceopen-parser/tests/fixtures/sccm/client/README.mdcrates/cmtraceopen-parser/tests/fixtures/sccm/client/intake/access-denied/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/client/intake/capped/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/client/intake/collision/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/client/intake/complete/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/client/intake/missing-root/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/client/intake/rotations/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/client/intake/rotations/manifest.jsoncrates/cmtraceopen-parser/tests/sccm_client_intake.rscrates/cmtraceopen-parser/tests/sccm_spine_contract.rsdocs/sccm/preparation/issue-319-client-intake.md
* test(sccm): define pure client intake contract * feat(sccm): define pure client intake coverage * test(sccm): harden client intake provenance boundaries * feat(sccm): validate client intake provenance * test(sccm): reject unsafe client intake provenance RED: cargo test --locked -p cmtraceopen-parser --test sccm_client_intake -- --nocapture ran 13 tests: 11 passed and 2 intended privacy regressions failed. * fix(sccm): bound client intake provenance paths * test(sccm): pin bounded intake path namespaces REVIEW RED: focused client intake ran 13 tests: 12 passed and the identity-slot bypass regression failed. * fix(sccm): allowlist client intake path namespaces * test(sccm): reject impossible intake timestamp paths CODERABBIT RED: focused client intake ran 13 tests: 12 passed and the impossible timestamp regression failed. * fix(sccm): validate intake timestamp path values * test(sccm): reject unnamespaced path fingerprints * fix(sccm): namespace client path fingerprints * test(sccm): reject identity-bearing synthetic handles * fix(sccm): bound synthetic fingerprint vocabulary * test(sccm): reject numeric synthetic identities * fix(sccm): constrain synthetic numeric markers * test(sccm): reject free-form intake metadata * fix(sccm): bound intake metadata vocabularies * test(sccm): reject free-form unknown rotations * fix(sccm): require opaque unknown rotations * test(sccm): close intake identity channels * fix(sccm): bind client intake identities * test(sccm): preserve shared location rotations * fix(sccm): bind shared location rotations * test(sccm): tighten client intake review regressions * fix(sccm): tighten client path fingerprint grammar * test(sccm): reject complete capped client fragments * fix(sccm): fail closed on complete capped fragments * test(sccm): expose escaped client path probe gap * test(sccm): harden client intake privacy probes * test(sccm): expose client intake coverage contradictions The client intake contract accepts three contradictions. A Captured fragment admitting fragmentComplete=false passes while the Capped mirror is rejected. A mixed group with one captured fragment and one Absent sibling marker reports group coverage Absent with a gap claiming no artifact was supplied, while the same group still serializes the captured fragment, because coverage_rank lets marker states outrank Captured and no test pins mixed-state groups. Two Absent markers for the same source under distinct caller labels are both accepted and project as two fragments because canonical-identity dedup only covers physical states. Add failing tests pinning the intended semantics: Captured plus incomplete fails closed via InvalidFragmentCompleteness, mixed groups keep the physical coverage and emit per-source gaps that name the absent or denied source, a capped mix keeps both the group gap and the per-source gap, and duplicate non-physical marker identities fail closed via DuplicateArtifactId. Refs #319 * fix(sccm): keep mixed-group client captures coherent Before this change the client intake accepted a Captured fragment that admitted fragmentComplete=false, let coverage_rank place Absent above Captured so a mixed group diagnosed itself as unsupplied while serializing the captured fragment, and let two non-physical markers double-declare the same source under distinct caller labels. Reject Captured plus incomplete via InvalidFragmentCompleteness, mirroring the Capped plus complete rule, since Capped exists to represent an incomplete capture. Compute group coverage from the physical fragments whenever any exist and emit an explicit per-source gap for each sibling marker that names the affected source, so absence stays a diagnosis without erasing captured evidence. Extend canonical-identity dedup to non-physical markers keyed on basename, rotation, and optional path fingerprint, mirroring the duplicate-identity idiom the server lane uses for physical states; markers distinguished by explicit fingerprints remain distinct sources. Also document that physicalArtifacts intentionally includes non-physical markers and that markers may carry a path fingerprint, both flagged by review as worth stating as intended. Refs #319 * test(sccm): probe forward-slash user-root leak detection The serialized privacy probe only matches the JSON-escaped backslash form c:\\users, so a forward-slash normalized leak such as c:/users/... with the RealUser sentinel stripped passes every serialized privacy assertion undetected. Add a failing meta-test that feeds the probe a forward-slash normalized Windows user path and requires detection, per the non-blocking hardening in the exact-head review. Refs #319 * fix(sccm): match normalized user roots in privacy probe The probe helper only recognized the JSON-escaped c:\\users form, leaving forward-slash normalized Windows user paths invisible to every serialized privacy assertion when the RealUser sentinel is absent. Also match the normalized c:/users form so either separator style trips the probe, closing the review's non-blocking hardening gap. Refs #319 * test(sccm): expose marker collisions with physical sources A fingerprint-less absent marker for a source that is also physically declared is accepted, and the assessment then claims no artifact for the source was supplied while serializing the captured fragment, the exact self-contradiction class round 6 blocked on. The same root lets an unpinned marker and a fingerprint-pinned marker double-declare one source, and lets a pinned marker contradict physical evidence for its own source. Add failing tests requiring the canonical source identity, basename plus rotation, to intersect across all declarations: a non-physical marker never shares basename and rotation with a physical declaration regardless of fingerprints, an unpinned marker collides with any other declaration for its source, and both directions hold for either declaration order. Pin the accepted direction, a marker for a genuinely distinct rotation alongside the capture, and pin the intentionally two-valued ParseFailed fragment completeness the review listed as the last unpinned matrix cell. Document the client/server fingerprint asymmetry as a follow-up. Refs #319 * fix(sccm): intersect source identity across declarations The marker identity set keyed on basename, rotation, and the optional path fingerprint never intersected physical declarations, so a fingerprint-less absent marker for a captured source was accepted and the assessment asserted a coverage gap its own fragments array disproved. The same root let an unpinned marker and a fingerprint-pinned marker double-declare one source, and let a pinned marker contradict physical evidence for its own source whenever the fingerprints differed. Track the canonical source identity, casefolded basename plus rotation discriminator, for every declaration and intersect it across all declaration shapes. A non-physical marker sharing its source identity with a physical declaration fails closed with CollidingPhysicalIdentity in either declaration order, regardless of fingerprints, because physical evidence disproves any absent, denied, or skipped claim about the same source. A fingerprint-less marker collides with any other declaration for its source via DuplicateArtifactId, while markers pinned to distinct roots by distinct fingerprints stay distinct sources when no physical declaration exists. Physical declarations keep their existing fingerprint and relative-path dedup, so distinct captured rotations and root collisions stay representable. The sibling server intake closes this class by making the path fingerprint mandatory everywhere; the client keeps optional marker fingerprints for the committed all-absent fixtures, with convergence documented as a follow-up in the intake tests. Refs #319 * test(sccm): pin exact client intake error variants Ten assertions checked only is_err(). SccmClientIntakeError has 14 variants, so each passed when any validator rejected the bundle, including for a reason unrelated to the behavior under test. Each now compares against the exact expected variant. Pinning exposed two inputs that rejected for the wrong reason: malformed_rotation_and_public_provenance_values_fail_closed declared SccmRotation::Timestamped("2026-bad") to exercise the rotation grammar, but synthetic_artifact derived an "unknown" group segment from the rotated basename. The bundle failed on InvalidRelativePath before reaching the rotation check, so the rotation contract was never exercised. The artifact now carries a consistent relative path and the malformed timestamp is the only contract it violates. The rotation grammar itself lives in the SccmRotation Serialize impl, which validate_bundle reaches through serde_json::to_value. fragment_completeness_and_every_path_fingerprint_are_explicit_and_ unambiguous asserted that an AccessDenied marker with no relative path fails its provenance contract. AccessDenied is not a physical state, so MissingPhysicalProvenance is unreachable for that input; the marker actually fails on InvalidFragmentCompleteness because it retains the default fragmentComplete=true. The assertion now pins that variant, and a new physical case with its relative path stripped covers MissingPhysicalProvenance directly. Refs #319 * refactor(sccm): own the sha256 digest width in one helper is_lowercase_hex_handle served both 16-character root handles and 64-character SHA-256 digests, so every digest caller had to restate its own length guard. Four call sites repeated it, and a new caller that omitted it would silently accept a 16-character handle. is_sha256_digest now owns the digest width and the four digest callers use it. is_lowercase_hex_handle keeps its 16-or-64 root grammar unchanged; narrowing the root contract would be a behavior change, not a refactor. Both delegate to a shared is_lowercase_hex. Refs #319 * test(sccm): casefold every serialized leak assertion unsupported_physical_artifacts_retain_safe_provenance_without_raw_ host_or_path compared against the original-case serialized JSON, so a projection that normalized case would leak RealUser or real-user-host and still pass. It also used bare substring checks, which miss the JSON-escaped and forward-slash normalized Windows user roots that carry no sentinel. Swept every serialized-leak assertion in the file. Two sites exist. The first already casefolded and routed through serialized_json_contains_windows_user_root; this commit brings the second to the same shape. Positive assertions keep their exact case on purpose, with a comment saying why, because they pin what the projection must reproduce verbatim. This class has now surfaced twice, so the contract is enforced rather than documented: the helper carries a doc comment stating the rule and a debug_assert that rejects a caller passing original-case input. Verified the guard fires by temporarily feeding it the uncased string. Siblings were checked and are not this class. sccm_client_intake_fixture_contract asserts a fixture lacks a [cut] marker, and sccm_spine_contract asserts redaction of parsed message strings, a surface that must retain C:\Windows\CCM\Logs verbatim. Refs #319 * fix(sccm): preserve partial client intake coverage * fix(sccm): repair client intake contracts (#432) * fix(sccm): repair client intake contracts (#319) * fix(sccm): bind multi-dot catalog requests (#319) * fix(sccm): bind and order client source lineages (#319) * fix(sccm): align client setup catalog identity (#319) * test(sccm): construct invalid requests without serialization * test(sccm): expose client intake wire gaps * fix(sccm): validate client intake wire projections * docs(sccm): align client intake delivery state * test(sccm): expose incomplete client intake oracle * test(sccm): bind complete client intake fixture oracle * docs(sccm): enumerate client coverage states * test(sccm): separate parse failure from fragment bounds * fix(sccm): distinguish parse failure from fragment bounds * docs(sccm): close client intake review nits * docs(sccm): clarify client intake fixture contracts * fix(sccm): stabilize client intake wire shape
Implements the reviewed pure-Rust #319 client intake and fixture-oracle slice on the current SCCM integration foundation.
Exact head:
269e713865cf51d578ef7edc7f0fa901163d0d90Reviewed integration base:
34c3abaa17baeb5c1f61e0254213bd017fdd7038Owned scope:
sccm::client::intakeAPI and exportsLocationServices.logcapture projected to both location and content consumersFixture matrix:
Selected TDD checkpoints:
44f97225/ GREEN1081ce67: initial pure intake contract4791a398/ GREENfd405bdf: provenance validation175461d2/ GREEN7cf05a09: mixed-group coverage coherence348d4251/ GREEN613416bc: marker/physical identity intersection73d2a1f0/ GREEN73eba644: fail-closed wire projections and canonical timestamps8d68ed99/ GREENf2f8bccc: exact typed fixture oracle5a870eaf: docs enumerate all seven SCCM coverage statese6373f31/ GREEN0284b8f8: separate parse normalization failure from fragment-boundary completeness5be9b954: close review tracking and fixture-format nits05d4bf0d: clarify manifest/oracle and coverage-order wording269e7138: stabilize explicit coverage-gap wire shape and review boundariesVerification at this exact head:
cargo test --locked -p cmtraceopen-parser --test sccm_client_intake --test sccm_client_intake_fixture_contract— 48 + 3 passedcmtraceopen-parsersuite — passedjq— passedgit diff --check— passed5a870eaf— PASS5be9b954— PASSsha2finding rejected becausesha2 = "0.11"already compiles in dev-dependencies5a870eaf..5be9b954— 0 findingsOut of scope and still open under #319:
This PR remains draft while fresh hosted CI, CodeRabbit, Copilot, and unresolved-thread checks run against the exact head. Issue #319 must remain open after the pure slice merges.
Issue: #319
Epic: #317
Summary by CodeRabbit
New Features
Documentation