test(sccm): prepare software update corpus - #351
Conversation
|
@coderabbitai review Please review exact head |
|
✅ Action performedReview finished.
|
|
Warning Review limit reached
Next review available in: 25 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (42)
📒 Files selected for processing (38)
📝 WalkthroughWalkthroughAdds a synthetic SCCM client updates corpus with 17 scenario manifests, expected contract outputs, rotation evidence, corpus documentation, and Rust tests validating metadata, physical evidence, deterministic hashing, parsing, correlation boundaries, and mutation rejection. ChangesSCCM client updates corpus
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (3)
crates/cmtraceopen-parser/tests/sccm_client_updates_fixture_contract.rs (3)
700-728: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
unwrap_or_default()weakens the marker assertions to prefix-only matches.If a counterpart fact field is missing or non-string, the marker degrades to e.g.
"CiId=", which the cited line satisfies for any CiId value — so a key mismatch between the fact and the evidence line would pass. Fail explicitly instead of substituting an empty value.🛡️ Proposed tightening
- let direct_markers = [ - format!( - "UpdateId={{{}}}", - fact["updateId"].as_str().unwrap_or_default() - ), + let key_fields = [ + ("updateId", "UpdateId={{{}}}"), + ("ciId", "CiId={}"), + // ... + ]; + // push a failure when any key field is absent/non-string, then build markers🤖 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_client_updates_fixture_contract.rs` around lines 700 - 728, Update the direct_markers construction in the SCCM fixture contract test to require each counterpart fact field to exist and be a string, failing the test when it is missing or invalid instead of using unwrap_or_default(). Preserve the existing marker comparisons while ensuring they always contain the fact’s actual value.
21-24: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHard-pinned corpus hash and exact capped bytes will fail opaquely on any legitimate fixture edit.
EXPECTED_CORPUS_FNV1A64andEXPECTED_CAPPED_CONTENTare intentional determinism gates, but the failure message ("evidence corpus drifted") gives no hint which file changed. Consider printing the per-file hashes on mismatch so future edits are diagnosable.Also applies to: 1134-1152
🤖 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_client_updates_fixture_contract.rs` around lines 21 - 24, Update the corpus-drift assertion around EXPECTED_CORPUS_FNV1A64 and EXPECTED_CAPPED_CONTENT to include per-file hashes when either deterministic check fails. Preserve the existing pinned values and validation behavior, but make the mismatch output identify each file and its computed hash so fixture changes are diagnosable.
364-376: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
sorted_stringsdoes not sort — the name inverts the actual contract.Both callers depend on that: Line 398 compares
stateChainagainstSTATE_CHAINpositionally (an order check), and Line 581 joinsprohibitedClaimsfor substring matching. A future reader "fixing" the helper to actually sort would silently void the state-chain ordering assertion.♻️ Rename to reflect behavior
-fn sorted_strings(values: &Value, field: &str) -> Vec<String> { +fn string_array(values: &Value, field: &str) -> Vec<String> {Update both call sites (
stateChain,prohibitedClaims) accordingly.Also applies to: 398-401, 581-581
🤖 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_client_updates_fixture_contract.rs` around lines 364 - 376, Rename sorted_strings to a name that reflects extraction without sorting, and update both callers that process stateChain and prohibitedClaims to use the new name. Preserve the helper’s current order-preserving behavior so the positional state-chain assertion remains effective.
🤖 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/updates/capped/expected.json`:
- Around line 40-45: Add the missing coverage entries for the captured artifact
families client-updates and client-location-services-shared to the expected
fixture, preserving the existing client-content=capped row and matching the
structure used by the validatedArtifactFamilies and updates-capped-02-sup
scenarios.
In
`@crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/incomplete/manifest.json`:
- Around line 52-59: Remove the rotation.fragmentComplete field from all three
captureState: absent artifacts in the fixture, while preserving their remaining
rotation data and leaving fragmentComplete unchanged for non-absent artifacts.
In
`@crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/no-sup/expected.json`:
- Around line 65-98: Update the expected transaction fixture for the locateSup
phase so supHostHandle is unavailable rather than the exact safe:sup:lab-sup-01
value when only updates-no-sup-01-scan evidence exists. Preserve the existing
insufficientEvidence classification and coverage-gap continuation, and follow
the shared unavailable-handle representation.
In
`@crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/no-sup/manifest.json`:
- Around line 52-59: Update the fixture entry for LocationServices.log so
artifacts with captureState "absent" omit rotation.fragmentComplete, while
preserving the remaining rotation metadata and the contract for captured
artifacts.
In
`@crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/success/manifest.json`:
- Around line 220-226: Remove the rotation.fragmentComplete field from the
skipped artifact identified by pathFingerprint
"synthetic:updates-success-08-supplemental-skipped". Keep captureState set to
"skipped" and preserve the remaining rotation fields and artifact metadata
unchanged.
In `@crates/cmtraceopen-parser/tests/sccm_client_updates_fixture_contract.rs`:
- Around line 1179-1181: Replace the self-comparison in the wrong_site mutation
case with an assertion against the manifest identity validator. Extract the
shared identity check used by the relevant manifest test into a helper such as
manifest_identity_failures, invoke it with wrong_site, and assert it reports the
siteCode drift while preserving the existing valid-manifest behavior.
In `@docs/sccm/preparation/issue-323-client-updates-corpus.md`:
- Line 204: Update the sentence beginning with “#323” in the issue-323
documentation to avoid a Markdown heading marker, using the requested “Issue
`#323`” wording or an escaped hash while preserving the sentence’s meaning.
---
Nitpick comments:
In `@crates/cmtraceopen-parser/tests/sccm_client_updates_fixture_contract.rs`:
- Around line 700-728: Update the direct_markers construction in the SCCM
fixture contract test to require each counterpart fact field to exist and be a
string, failing the test when it is missing or invalid instead of using
unwrap_or_default(). Preserve the existing marker comparisons while ensuring
they always contain the fact’s actual value.
- Around line 21-24: Update the corpus-drift assertion around
EXPECTED_CORPUS_FNV1A64 and EXPECTED_CAPPED_CONTENT to include per-file hashes
when either deterministic check fails. Preserve the existing pinned values and
validation behavior, but make the mismatch output identify each file and its
computed hash so fixture changes are diagnosable.
- Around line 364-376: Rename sorted_strings to a name that reflects extraction
without sorting, and update both callers that process stateChain and
prohibitedClaims to use the new name. Preserve the helper’s current
order-preserving behavior so the positional state-chain assertion remains
effective.
🪄 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: c8facd8d-71f4-45dc-bae5-973c1a763124
⛔ Files ignored due to path filters (42)
crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/access-denied/evidence/client-updates/current/ScanAgent.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/capped/evidence/client-content/current/DataTransferService.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/capped/evidence/client-location-services-shared/current/LocationServices.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/capped/evidence/client-updates/current/ScanAgent.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/content-failure/evidence/client-content/current/ContentTransferManager.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/content-failure/evidence/client-location-services-shared/current/LocationServices.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/content-failure/evidence/client-updates/current/ScanAgent.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/evaluation-failure/evidence/client-updates/current/ScanAgent.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/evaluation-failure/evidence/client-updates/current/WUAHandler.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/incomplete/evidence/client-updates/current/UpdatesDeployment.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/install-failure/evidence/client-content/current/DataTransferService.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/install-failure/evidence/client-location-services-shared/current/LocationServices.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/install-failure/evidence/client-updates/current/ScanAgent.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/install-failure/evidence/client-updates/current/UpdatesHandler.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/invalid-offset/evidence/client-updates/current/ScanAgent.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/invalid-offset/evidence/client-updates/current/UpdatesStore.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/maintenance-window/evidence/client-content/current/DataTransferService.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/maintenance-window/evidence/client-location-services-shared/current/LocationServices.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/maintenance-window/evidence/client-maintenance-window/current/ServiceWindowManager.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/maintenance-window/evidence/client-updates/current/ScanAgent.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/malformed/evidence/client-updates/current/ScanAgent.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/no-sup/evidence/client-updates/current/ScanAgent.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/reboot-pending/evidence/client-location-services-shared/current/LocationServices.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/reboot-pending/evidence/client-reboot/current/RebootCoordinator.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/reboot-pending/evidence/client-updates/current/ScanAgent.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/reboot-pending/evidence/client-updates/current/UpdatesDeployment.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/reporting-failure/evidence/client-location-services-shared/current/LocationServices.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/reporting-failure/evidence/client-policy-state/current/StateMessage.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/reporting-failure/evidence/client-updates/current/ScanAgent.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/reporting-failure/evidence/client-updates/current/UpdatesDeployment.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/rotation-boundary/evidence/client-updates/current/ScanAgent.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/same-minute-separate/evidence/client-updates/current/UpdatesHandler.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/scan-failure/evidence/client-updates/current/ScanAgent.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/success/evidence/client-content/current/DataTransferService.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/success/evidence/client-location-services-shared/current/LocationServices.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/success/evidence/client-maintenance-window/current/ServiceWindowManager.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/success/evidence/client-policy-state/current/StateMessage.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/success/evidence/client-reboot/current/RebootCoordinator.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/success/evidence/client-updates/current/ScanAgent.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/success/evidence/client-updates/current/UpdatesHandler.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/supplemental-conflict/evidence/client-updates/current/UpdatesHandler.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/supplemental-conflict/evidence/client-windows-update-supplemental/current/CBS.logis excluded by!**/*.log
📒 Files selected for processing (38)
crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/README.mdcrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/access-denied/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/access-denied/manifest.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/capped/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/capped/manifest.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/content-failure/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/content-failure/manifest.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/evaluation-failure/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/evaluation-failure/manifest.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/incomplete/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/incomplete/manifest.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/install-failure/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/install-failure/manifest.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/invalid-offset/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/invalid-offset/manifest.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/maintenance-window/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/maintenance-window/manifest.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/malformed/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/malformed/manifest.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/no-sup/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/no-sup/manifest.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/reboot-pending/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/reboot-pending/manifest.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/reporting-failure/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/reporting-failure/manifest.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/rotation-boundary/evidence/client-updates/lo/ScanAgent.lo_crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/rotation-boundary/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/rotation-boundary/manifest.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/same-minute-separate/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/same-minute-separate/manifest.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/scan-failure/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/scan-failure/manifest.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/success/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/success/manifest.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/supplemental-conflict/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/supplemental-conflict/manifest.jsoncrates/cmtraceopen-parser/tests/sccm_client_updates_fixture_contract.rsdocs/sccm/preparation/issue-323-client-updates-corpus.md
Independent exact-range review — BLOCKEDReview anchor: base Verified on the exact range
Blocking findings
CodeRabbit state (not approval)
PR must remain draft/open and unmerged until these blockers are fixed on the restacked head and both CodeRabbit and fresh independent review are complete. |
CodeRabbit completion / thread disposition on
|
299f9a1 to
4d275af
Compare
Review remediation on exact restackReview anchor: base The original #323 patch was restacked without semantic drift ( TDD and review fixes
Verification
This remains preparation-only: no production reducer, native collection, server diagnosis, or correlation implementation, and no live Windows acceptance claim. PR stays draft/open pending fresh exact-head CodeRabbit and independent rereview. |
|
@coderabbitai review Please run a fresh review of the exact restacked range:
Please include the review-fix commit and re-evaluate the previously actionable threads. The |
|
✅ Action performedReview finished.
|
4d275af to
6784259
Compare
Exact anchor advanced after #350 mergeThis supersedes the prior Current exact range: base Range-diff preserved both prior #323 patches exactly:
The new regression followed RED -> GREEN. It exercises the public Fresh verification on
Whole-repository PR remains draft/open with no production reducer, native acceptance, server cause, or correlation claim. Fresh CodeRabbit and independent rereview must use the current exact anchor above. |
|
@coderabbitai review Fresh exact-range request after the shared #350 merge:
Please re-review all current changes, including the experimental-Low-key noncausal regression in |
|
✅ Action performedReview finished.
|
adamgell
left a comment
adamgell
left a comment
There was a problem hiding this comment.
Independent read-only adversarial rereview of exact range 25b37333affde22b4ef8a19f4a5f3d89c082b599..6784259fa031ce2287934317b77afe665ae5fe44: BLOCKED.
Disposable in-memory probes invoked the real scenario_semantic_failures path. It correctly rejected foreign citations; wrong kind/source version/profile; extra, conflicting, or missing coverage and gaps; absent/skipped fragmentComplete; invented no-SUP handles; SUP key/profile/site/timestamp/offset/ordering drift; time-only/prefix facts; supplemental or invalid-offset High escalation; same-minute outcome/key swaps; wrong site; and experimental 5.00.9128 Low-to-exact/high promotion.
Four fail-closed holes remain:
- A success transaction can remain Report/High while citing only its Scan and LocateSup records, and an Install High
confirmedFailureplus its finding can cite only Scan and LocateSup. Both full semantic validations returned zero failures; phase outcome and terminality are not derived from cited source-specific records. - Exact transaction fields are accepted independently across the cited record set. A same-minute transaction combining UpdateId from line 1 with CI/content/job/client values from line 2 passed
transaction_binding_failures; the full validator rejected only the changed evidence range through the hard-coded same-minute snapshot, not because the key never co-occurred. - The semantic validator accepted duplicate group membership, a second artifact aliasing the same relativePath/pathFingerprint, an artifact role changed to server, a catalog entry/profile-family substitution, and a report artifact redirected to the scan path. Corpus-wide counts catch the committed corpus, but the advertised semantic mutation gate is not fail-closed.
- A counterpart fact carrying
topologyCompatible: falseremainedcorrelationEligible: truewith zero failures.
Independent corpus recomputation: 17 scenarios, 51 artifacts, states 42 captured / 4 absent / one each accessDenied, capped, parseFailed, skipped, unsupported; 43 declared and physical files; 23,142 bytes; 61 lines; 57 complete CCM records; path-qualified SHA-256 b7670821f385f90eb0178528480307f617c508c28abacf21e927d30ed3bdffef; capped 128-byte SHA-256 a0afd1fa4e1204c6d085886ed62b07f6b1d4af119f747c181f6e206194db9f7f; no missing/orphan/privacy hits. Current+lo and lo+current each produce 0 CCM records and 2 parse errors. Scope remains fixture/test/spec-only with no production reducer, native adapter, server cause, closing keyword, or live Windows claim.
Fresh gates: focused 5/5; spine 61/61; deployment 8/8; intake 3/3; full parser 659/659; strict Clippy; wasm32; TypeScript; scoped Rust 1.88 format; exact-range diff check. Range-diff preserves the two earlier patches and adds only 6784259.
CodeRabbit is in the loop but has no substantive approval for this exact head: the GitHub exact-head response is only an incremental acknowledgement, the last substantive approval is stale at 4d275af, 42 log files remain path-filtered, and authenticated CLI 0.7.1 returned a recoverable 34-minute rate limit. Do not mark ready or merge until these semantic blockers are fixed, exact-head CodeRabbit is substantive, and fresh independent rereview passes.
Independent-review remediation — exact head
|
|
@coderabbitai full review Please perform a substantive full review from scratch on the current exact range:
Include commit |
|
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 14 minutes. |
adamgell
left a comment
adamgell
left a comment
There was a problem hiding this comment.
Independent read-only adversarial rereview of exact range 25b37333affde22b4ef8a19f4a5f3d89c082b599..89a87b54648223839bec126a8ee5cbfa59575c82: BLOCKED.
The corrected real validator now rejects every previously demonstrated fail-open mutation on its intended path: Report/High without Report evidence; Install/High confirmedFailure without a terminal source record; cross-record exact-key chimeras; duplicate group membership and physical relativePath/pathFingerprint aliases; client-role, catalog/group, basename/path, profile-family, and rotation drift; report-to-scan redirection; literal topologyCompatible:false plus eligibility; and experimental-profile promotion. The complete prior rejected matrix also remains rejected.
Two new fail-closed holes remain:
- A captured physical artifact can set
pathFingerprintto null (or omit it) and the fullscenario_semantic_failurespath returns zero failures. #319 defines physical identity from configured-root/path fingerprint plus basename/rotation, and #323 promises collision-safe path/fingerprint provenance. Uniqueness checks that skip non-string values do not establish that every physical artifact has a non-empty fingerprint. - A counterpart fact can add
topologyCompatible:truewhile the enclosing handoff still saystopologyCompatibilityEvaluated:false, leavecorrelationEligible:true, and the full semantic validator returns zero failures. The new guard handles only literal false. This permits unsupported client-side topology compatibility self-attestation even though the corpus contract says topology is not evaluated here. Reject an unevaluated compatibility claim (and malformed present values), not only false+eligible.
Disposable probes invoked the private validator through a temporary integration-test include and were removed afterward; the detached review worktree is clean.
Independent corpus recomputation: 17 scenarios; 51 artifacts; states 42 captured / 4 absent / one each accessDenied, capped, parseFailed, skipped, unsupported; 43 declared and physical files; 23,142 bytes; 61 lines; 2 partial and 1 capped; no missing/orphan files; path-qualified SHA-256 b7670821f385f90eb0178528480307f617c508c28abacf21e927d30ed3bdffef. The focused parser contract also pins 57 complete CCM records.
Fresh gates: focused 10/10; spine 61/61; deployment 8/8; intake 3/3; full parser 664/664; strict Clippy; wasm32 check; TypeScript; scoped Rust 1.88 format; exact-range diff check. Scope remains fixture/test/spec-only with no production reducer, native adapter, server cause, closing keyword, or live Windows acceptance claim.
CodeRabbit is in the loop, but the fresh exact-head request currently has only an acknowledgement; no substantive exact-head result/review submission exists yet. Do not mark ready or merge until both blockers are fixed, exact-head CodeRabbit is substantive, and a fresh independent rereview passes.
|
Exact-head remediation for the independent BLOCKED review at #351 (review) is pushed. Commits:
TDD evidence:
Fresh verification on the committed content:
Independent corpus recomputation remains 17 scenarios, 51 artifacts, 43 physical files, 23,142 bytes, 61 lines; path-qualified SHA-256 All 11 review threads are resolved. This PR remains open and draft for a fresh independent rereview. No production reducer, native Windows acceptance, server cause, or cross-side correlation is claimed. |
|
@coderabbitai full review Please perform a fresh full review of exact head The authenticated CodeRabbit CLI completed substantive correction-range reviews:
Keep this draft PR open for independent rereview; do not treat the draft-skipped status check as a substantive verdict. |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/cmtraceopen-parser/tests/sccm_client_updates_fixture_contract.rs (1)
602-605: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReturn a failure for a non-array
counterpartReadyFactsinstead of panicking.Every other shape error in
expected_boundary_failurespushes a failure and returns. Line 603 uses.expect, so a non-arraycounterpartReadyFactspanics the validator. The shape mutation table at lines 3369-3402 does not cover this field, so the panic path is untested. The same concern applies tojson_string(line 380) andstring_array(line 391), which panic when called at lines 501, 535, 539, 543, 586-587 and 696 with non-string JSON.♻️ Proposed fail-closed handling
let handoff = &expected["correlationHandoff"]; - let facts = handoff["counterpartReadyFacts"] - .as_array() - .expect("counterpartReadyFacts must be an array"); + let Some(facts) = handoff["counterpartReadyFacts"].as_array() else { + failures.push(format!( + "{scenario}: counterpartReadyFacts must be an array" + )); + return failures; + };You can then add a
counterpartReadyFacts objectrow toshape_mutationswith the marker"counterpartReadyFacts must be an array".🤖 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_client_updates_fixture_contract.rs` around lines 602 - 605, Replace the `.expect` on `handoff["counterpartReadyFacts"]` in `expected_boundary_failures` with fail-closed handling that pushes the existing validation failure and returns when the value is not an array. Apply the same non-panicking pattern to the `json_string` and `string_array` helpers, preserving their existing error markers and callers’ behavior, then add the `counterpartReadyFacts object` mutation case to `shape_mutations` using the marker `counterpartReadyFacts must be an array`.
🤖 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/updates/supplemental-conflict/expected.json`:
- Around line 96-119: Update the supplemental-conflict expected fixture so the
unkeyed CBS source-local observation’s lastSuccessfulPhase is null, and make the
corresponding finding’s lastSuccessfulPhase null as well. Preserve the keyed
client transaction as the only observation reporting install success, without
changing its value or other evidence.
---
Nitpick comments:
In `@crates/cmtraceopen-parser/tests/sccm_client_updates_fixture_contract.rs`:
- Around line 602-605: Replace the `.expect` on
`handoff["counterpartReadyFacts"]` in `expected_boundary_failures` with
fail-closed handling that pushes the existing validation failure and returns
when the value is not an array. Apply the same non-panicking pattern to the
`json_string` and `string_array` helpers, preserving their existing error
markers and callers’ behavior, then add the `counterpartReadyFacts object`
mutation case to `shape_mutations` using the marker `counterpartReadyFacts must
be an array`.
🪄 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: f60615ba-10b2-45a2-95a3-3da5cf92605d
⛔ Files ignored due to path filters (42)
crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/access-denied/evidence/client-updates/current/ScanAgent.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/capped/evidence/client-content/current/DataTransferService.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/capped/evidence/client-location-services-shared/current/LocationServices.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/capped/evidence/client-updates/current/ScanAgent.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/content-failure/evidence/client-content/current/ContentTransferManager.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/content-failure/evidence/client-location-services-shared/current/LocationServices.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/content-failure/evidence/client-updates/current/ScanAgent.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/evaluation-failure/evidence/client-updates/current/ScanAgent.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/evaluation-failure/evidence/client-updates/current/WUAHandler.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/incomplete/evidence/client-updates/current/UpdatesDeployment.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/install-failure/evidence/client-content/current/DataTransferService.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/install-failure/evidence/client-location-services-shared/current/LocationServices.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/install-failure/evidence/client-updates/current/ScanAgent.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/install-failure/evidence/client-updates/current/UpdatesHandler.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/invalid-offset/evidence/client-updates/current/ScanAgent.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/invalid-offset/evidence/client-updates/current/UpdatesStore.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/maintenance-window/evidence/client-content/current/DataTransferService.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/maintenance-window/evidence/client-location-services-shared/current/LocationServices.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/maintenance-window/evidence/client-maintenance-window/current/ServiceWindowManager.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/maintenance-window/evidence/client-updates/current/ScanAgent.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/malformed/evidence/client-updates/current/ScanAgent.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/no-sup/evidence/client-updates/current/ScanAgent.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/reboot-pending/evidence/client-location-services-shared/current/LocationServices.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/reboot-pending/evidence/client-reboot/current/RebootCoordinator.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/reboot-pending/evidence/client-updates/current/ScanAgent.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/reboot-pending/evidence/client-updates/current/UpdatesDeployment.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/reporting-failure/evidence/client-location-services-shared/current/LocationServices.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/reporting-failure/evidence/client-policy-state/current/StateMessage.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/reporting-failure/evidence/client-updates/current/ScanAgent.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/reporting-failure/evidence/client-updates/current/UpdatesDeployment.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/rotation-boundary/evidence/client-updates/current/ScanAgent.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/same-minute-separate/evidence/client-updates/current/UpdatesHandler.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/scan-failure/evidence/client-updates/current/ScanAgent.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/success/evidence/client-content/current/DataTransferService.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/success/evidence/client-location-services-shared/current/LocationServices.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/success/evidence/client-maintenance-window/current/ServiceWindowManager.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/success/evidence/client-policy-state/current/StateMessage.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/success/evidence/client-reboot/current/RebootCoordinator.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/success/evidence/client-updates/current/ScanAgent.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/success/evidence/client-updates/current/UpdatesHandler.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/supplemental-conflict/evidence/client-updates/current/UpdatesHandler.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/supplemental-conflict/evidence/client-windows-update-supplemental/current/CBS.logis excluded by!**/*.log
📒 Files selected for processing (38)
crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/README.mdcrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/access-denied/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/access-denied/manifest.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/capped/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/capped/manifest.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/content-failure/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/content-failure/manifest.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/evaluation-failure/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/evaluation-failure/manifest.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/incomplete/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/incomplete/manifest.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/install-failure/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/install-failure/manifest.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/invalid-offset/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/invalid-offset/manifest.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/maintenance-window/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/maintenance-window/manifest.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/malformed/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/malformed/manifest.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/no-sup/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/no-sup/manifest.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/reboot-pending/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/reboot-pending/manifest.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/reporting-failure/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/reporting-failure/manifest.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/rotation-boundary/evidence/client-updates/lo/ScanAgent.lo_crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/rotation-boundary/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/rotation-boundary/manifest.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/same-minute-separate/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/same-minute-separate/manifest.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/scan-failure/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/scan-failure/manifest.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/success/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/success/manifest.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/supplemental-conflict/expected.jsoncrates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/supplemental-conflict/manifest.jsoncrates/cmtraceopen-parser/tests/sccm_client_updates_fixture_contract.rsdocs/sccm/preparation/issue-323-client-updates-corpus.md
adamgell
left a comment
adamgell
left a comment
There was a problem hiding this comment.
Review of PR #351 at b15235b (independent re-review after the prior BLOCK of 971563f). Verdict: BLOCK.
Verification battery (all run at the exact head, --locked): merge-base with foundation a8a367c confirmed; sccm_client_updates_fixture_contract 13 passed; full cmtraceopen-parser suite 742 passed; clippy with -D warnings clean; wasm32-unknown-unknown check clean.
The three substantive corrections hold under independent adversarial probing. Removing the phase-disposition record for maintenance-window or reboot-pending fails the contract, as does an install-failure claim without its cited terminal and maintenanceWindow evidence; lastSuccessfulPhase mutations to unproven phases, to phases backed only by source-version-incompatible artifacts, and to null where the contract pins a proven phase are all rejected. ReportingEvents.log retyping to a CCM source or a non-null sourceVersion is rejected. The pathFingerprint and sanitizedSourcePath grammars reject UNC paths, email addresses, uppercase prefix variants, over-long opaque segments, control characters, foreign domains, and drive paths, verified both in memory and by mutating the serialized JSON fixture on disk. The diff introduces no privacy-sensitive material.
Two issues block approval:
-
The panic-free claim is only true for the shapes the new test enumerates. Seven adjacent malformed-shape mutations still panic the validator: counterpartReadyFacts as an object, correlationHandoff as a scalar, non-string transactionId, non-string finding subjectId, non-string transaction state, non-array prohibitedClaims, and string coverageGapArtifactIds. The panic sites are the expect at line 604 and the json_string and string_array helpers (lines 380 and 391) called on caller-controlled JSON at lines 501, 543, 557, 645, and 1221 of the contract test. Panics still fail CI, so no mutation is silently accepted, but the stated contract of the fix is not met. Please route these through the same failure-string path and extend the shape-mutation test to cover them.
-
The prep doc added in this head states that CBS.log carries no ConfigMgr sourceVersion, but supplemental-conflict ships CBS.log with sourceVersion 5.00.TEST.0000 and the validator constrains sourceVersion to null only for supplementalLog. Both null and non-null values pass today. Please null the fixture value and enforce it for cbsLog as well.
Additionally, three defect classes that blocked sibling lanes reproduce here and should be closed in this PR for program parity: duplicate citation tuples validate clean, overlapping citation line ranges validate clean (the class that blocked #375), and absent artifacts may carry stale encoding and collectionLimit metadata which the provenance projection nulls instead of rejecting (the class that blocked #352). Unknown-field schema closure remains a program-wide design decision and is not asked of this PR.
All probes were reverted; the worktree is clean at b15235b and the full battery passes 742 tests.
Before this change the panic-free shape battery covered only the enumerated shapes. Seven adjacent malformed-shape mutations still panic the validator instead of failing closed: counterpartReadyFacts as an object, correlationHandoff as a scalar, non-string transactionId, non-string finding subjectId, non-string transaction state, non-array prohibitedClaims, and string coverageGapArtifactIds. The panic sites are the counterpartReadyFacts expect and the json_string and string_array helpers called on caller-controlled JSON. Extend the shape-mutation battery with all seven shapes and add direct helper-level probes proving the helpers never panic, so the whole class is closed rather than an enumeration. Refs #323
Before this change the json_string and string_array helpers and the counterpartReadyFacts expect panicked when caller-controlled JSON carried a wrong shape, so seven adjacent malformed-shape mutations crashed the validator instead of producing failure strings. Convert both helpers to return failure strings and route every call site (transaction and observation ids, finding subjectId, subject outcome fields, coverage entries, prohibited claims, same-minute update keys, coverage gap ids, manifest artifact ids, and the counterpart facts array) through the accumulated-failure path, so the entire malformed-shape class fails closed rather than only the enumerated shapes. Refs #323
Before this change the prep doc stated that CBS.log carries no ConfigMgr sourceVersion, yet the supplemental-conflict fixture shipped CBS.log with sourceVersion 5.00.TEST.0000 and the validator constrained sourceVersion to null only for the supplementalLog kind, so both null and non-null CBS values passed. Add a test proving a cbsLog artifact with a ConfigMgr sourceVersion is rejected by kind validation and by the full scenario validator, and pinning the shipped fixture value to null. Refs #323
Before this change the supplemental-conflict fixture shipped CBS.log with a ConfigMgr sourceVersion of 5.00.TEST.0000 while the prep doc states neither CBS.log nor ReportingEvents.log carries one, and the validator forced null only for the supplementalLog kind. Key the null-sourceVersion constraint on the basename-derived expected kind so both cbsLog and supplementalLog artifacts reject any ConfigMgr sourceVersion, and null the shipped CBS.log fixture value so the corpus matches the documented supplemental servicing boundary. Refs #323
Before this change the supplemental-conflict source-local CBS observation carried key null with keyConfidence none yet claimed lastSuccessfulPhase install, and its finding mirrored the claim. The prep doc requires every non-null lastSuccessfulPhase to be proven by a compatible cited complete record containing the exact key, which an unkeyed observation can never satisfy, but the validator accepted the claim. Add a test proving a null-key observation claiming lastSuccessfulPhase is rejected and pinning the shipped observation and finding values to null. Refs #323
Before this change a source-local observation with key null could carry a non-null lastSuccessfulPhase even though the corpus contract requires every non-null lastSuccessfulPhase to be proven by a cited complete record containing the exact key, which an unkeyed CBS symptom can never provide. Enforce null lastSuccessfulPhase on every keyless observation, null the supplemental-conflict observation and finding claims, and update the scenario contract so the keyed client transaction remains the only install-success result. Refs #323
Before the malformed-shape closure a scalar counterpartReadyFacts value panicked the validator at the counterpart facts expect, the same site as the object variant. The closure commit already routes every non-array counterpartReadyFacts value through the failure string path; add the scalar variant to the shape battery explicitly so object, scalar, and scalar-handoff forms are each pinned by a named mutation. Refs #323
Before this change evidence citations were only bounds-checked per tuple, so duplicating a transaction evidence citation in the success scenario, adding an overlapping updates-success-01-scan line 1-1 range beside the existing 1-2 range, and duplicating the supplemental-conflict observation citation all validated clean, double-counting the same logical CCM record for chronology and corroboration. Add a test proving each mutation is rejected, the class that blocked sibling lanes #326 and #375. Refs #323
Before this change citation validation only bounds-checked each tuple, so duplicate tuples and overlapping line ranges could cite the same logical CCM record twice within one evidence list and inflate chronology or corroboration. Expand every in-bounds cited range into (artifactId, line) identities with the same evidence_refs_cite_unique_records helper sibling lane #326 uses and fail closed inside citation_failures, closing the class for transaction, observation, finding, and counterpart citations alike. Refs #323
Before this change an absent or access-denied artifact could carry encoding, collectionLimit, or truncated metadata and still validate clean because artifact_provenance_projection silently nulled the fields instead of the validator rejecting them, letting noncapture artifacts retain stale physical provenance. Add a test proving each field is rejected on an absent artifact in the incomplete scenario and on the access-denied artifact, the class that blocked sibling lanes #324 and #352. Refs #323
Before this change the noncapture manifest branch only rejected relativePath and nonzero bytesCopied, so absent, access-denied, skipped, unsupported, and parse-failed artifacts could carry stale encoding, collectionLimit, or truncated metadata that the provenance projection silently nulled. Reject each physical metadata field on every noncapture capture state so stale provenance fails closed, mirroring the sibling lane #324 noncapture idiom. Refs #323
Before this change the malformed-shape helper probe vector used an inline tuple type that trips the clippy type_complexity gate under -D warnings. Name the probe tuple with a lifetime-parameterized type alias; behavior is unchanged. Refs #323
Before this change five constructs added during the review-fix round drifted from rustfmt layout in the updates contract test. Run rustfmt on that single changed file so the format gate passes; no behavioral change. Refs #323
|
@coderabbitai full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 25 minutes. |
adamgell
left a comment
adamgell
left a comment
There was a problem hiding this comment.
Round 6 review at exact head a1d34a1.
Reviewed in a detached exact-head worktree; no commits, pushes, or
configuration changes were made, and the tree ended clean.
Verdict: PASS.
Battery
- sccm_client_updates_fixture_contract: 17 passed (expected 17), with
zero "panicked" occurrences under RUST_BACKTRACE=0. - sccm_spine_contract: 136 passed (expected 136).
- cargo test -p cmtraceopen-parser: 746 passed (expected 746).
- cargo test --workspace: 1358 passed, 0 failed.
- cargo clippy --workspace --all-targets -- -D warnings: clean.
- cargo check -p cmtraceopen-parser --target wasm32-unknown-unknown: clean.
- Descent: git merge-base HEAD a8a367c prints a8a367c, confirming the
documented foundation.
Shape class closure
The round 5 blocker asked whether panic freedom held beyond the
enumerated shapes. It does. Rather than replay the named shapes, this
review ran a recursive mutator over every JSON pointer path in all 17
manifest and expected fixtures, substituting 12 adversarial value
classes including null where an object is expected, deeply nested wrong
types, arrays of wrong element type, u64::MAX, i64::MIN, a huge float
where a string is expected, and whole-document replacement. Results:
46,800 probes against scenario_semantic_failures and 507,000 probes
across all 13 validator helpers, with zero panics. The property is
structural: serde_json read indexing yields Null rather than panicking,
so the helper reads cannot panic on shape. The remaining expect sites
are fixture wellformedness preconditions in test bodies, not
caller-controlled shape paths. Citation range expansion is also
protected from unbounded iteration because the bounds check precedes it.
Claim adjudication
- cbsLog and supplementalLog non-null sourceVersion are both rejected.
All three CBS.log and ReportingEvents.log artifacts in the corpus now
carry null sourceVersion, matching the revised documentation. The
round 5 doc versus fixture contradiction is resolved. - A null-key observation carrying a non-null lastSuccessfulPhase is
rejected, and a keyed transaction with a valid phase claim still
passes, so the fix does not over-reject. In supplemental-conflict the
keyed transaction remains the only install success, which closes the
separately raised Major on the unkeyed CBS observation. - Duplicate citation tuples, nested overlaps, and a fully in-bounds
three-way overlap are all rejected via (artifactId, line) expansion. - Noncapture artifacts reject encoding, collectionLimit, and truncated
on both absent and accessDenied. - Rounds 1 through 4 closures hold: phase disposition evidence for
maintenance-window and reboot-pending, lastSuccessfulPhase evidence
binding, ReportingEvents supplementalLog typing, and fingerprint
grammar for both the JSON-escaped and forward-slash path forms. - RED/GREEN discipline is intact: each RED commit touches only the
contract test, and each GREEN commit carries the corresponding
validator, fixture, or documentation change.
Observations, none blocking
- ccmLog sourceVersion is enforced at family granularity rather than
per artifact. Nulling one member of the two-member client-updates
family is absorbed, while nulling both is caught by family projection
drift. This matches the documented family-level projection, so it is
a granularity property of the stated contract rather than a gap. - manifest_artifact_kind_failures in isolation accepts a ccmLog placed
in the supplemental group, but the composed validator rejects that
input three independent ways, so defense in depth holds. - Cross-subject citation duplication is deliberately not deduplicated,
since a transaction and its finding legitimately cite the same
evidence.
Privacy
A sweep of the 971563f..HEAD diff found no IP addresses, email
addresses, UNC paths, or real user directories. The only user-profile
strings present are fabricated negative vectors asserting that such
paths fail closed.
There was a problem hiding this comment.
Pull request overview
This PR adds a spec/fixture-only synthetic corpus for the SCCM client software-update workflow (issue #323) to pin evidence-first boundaries and expected conservative outcomes before any production reducer or native capture work lands.
Changes:
- Adds a 17-scenario synthetic fixture corpus (manifests, evidence, expected outputs) for client updates.
- Adds a Rust contract test (
sccm_client_updates_fixture_contract.rs) that validates corpus identity (counts/hashes), CCM framing, evidence citations, and correlation handoff constraints. - Adds preparation documentation describing the intended state machine, evidence/key requirements, and boundary conditions.
Reviewed changes
Copilot reviewed 37 out of 80 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| docs/sccm/preparation/issue-323-client-updates-corpus.md | Documents the proposed evidence-first contract and scenario matrix for the client updates workflow |
| crates/cmtraceopen-parser/tests/sccm_client_updates_fixture_contract.rs | Adds the contract test enforcing corpus structure, invariants, and identity hashes |
| crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/README.md | Describes corpus purpose, synthetic-data boundaries, and replay commands |
| crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/access-denied/expected.json | Expected output for access-denied scenario |
| crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/access-denied/manifest.json | Manifest for access-denied scenario |
| crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/access-denied/evidence/client-updates/current/ScanAgent.log | Synthetic evidence for access-denied scenario |
| crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/capped/expected.json | Expected output for capped scenario |
| crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/capped/manifest.json | Manifest for capped scenario |
| crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/capped/evidence/client-content/current/DataTransferService.log | Synthetic evidence for capped scenario (capped content) |
| crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/capped/evidence/client-location-services-shared/current/LocationServices.log | Synthetic evidence for capped scenario (LocateSup) |
| crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/capped/evidence/client-updates/current/ScanAgent.log | Synthetic evidence for capped scenario (Scan/Evaluate) |
| crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/content-failure/expected.json | Expected output for content-failure scenario |
| crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/content-failure/manifest.json | Manifest for content-failure scenario |
| crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/content-failure/evidence/client-content/current/ContentTransferManager.log | Synthetic evidence for content-failure scenario (Download failure) |
| crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/content-failure/evidence/client-location-services-shared/current/LocationServices.log | Synthetic evidence for content-failure scenario (LocateSup) |
| crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/content-failure/evidence/client-updates/current/ScanAgent.log | Synthetic evidence for content-failure scenario (Scan/Evaluate) |
| crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/evaluation-failure/expected.json | Expected output for evaluation-failure scenario |
| crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/evaluation-failure/manifest.json | Manifest for evaluation-failure scenario |
| crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/evaluation-failure/evidence/client-updates/current/ScanAgent.log | Synthetic evidence for evaluation-failure scenario (Scan) |
| crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/evaluation-failure/evidence/client-updates/current/WUAHandler.log | Synthetic evidence for evaluation-failure scenario (Evaluate failure) |
| crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/incomplete/expected.json | Expected output for incomplete scenario |
| crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/incomplete/manifest.json | Manifest for incomplete scenario |
| crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/incomplete/evidence/client-updates/current/UpdatesDeployment.log | Synthetic evidence for incomplete scenario (partial chain) |
| crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/install-failure/expected.json | Expected output for install-failure scenario |
| crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/install-failure/manifest.json | Manifest for install-failure scenario |
| crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/install-failure/evidence/client-content/current/DataTransferService.log | Synthetic evidence for install-failure scenario (Download) |
| crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/install-failure/evidence/client-location-services-shared/current/LocationServices.log | Synthetic evidence for install-failure scenario (LocateSup) |
| crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/install-failure/evidence/client-updates/current/ScanAgent.log | Synthetic evidence for install-failure scenario (Scan/Evaluate) |
| crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/install-failure/evidence/client-updates/current/UpdatesHandler.log | Synthetic evidence for install-failure scenario (Install failure) |
| crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/invalid-offset/expected.json | Expected output for invalid-offset scenario |
| crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/invalid-offset/manifest.json | Manifest for invalid-offset scenario |
| crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/invalid-offset/evidence/client-updates/current/ScanAgent.log | Synthetic evidence for invalid-offset scenario (Scan) |
| crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/invalid-offset/evidence/client-updates/current/UpdatesStore.log | Synthetic evidence for invalid-offset scenario (invalid offset) |
| crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/malformed/expected.json | Expected output for malformed scenario |
| crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/malformed/manifest.json | Manifest for malformed scenario |
| crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/malformed/evidence/client-updates/current/ScanAgent.log | Synthetic evidence for malformed scenario (malformed key) |
| crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/maintenance-window/expected.json | Expected output for maintenance-window scenario |
| crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/maintenance-window/manifest.json | Manifest for maintenance-window scenario |
| crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/maintenance-window/evidence/client-content/current/DataTransferService.log | Synthetic evidence for maintenance-window scenario (Download) |
| crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/maintenance-window/evidence/client-location-services-shared/current/LocationServices.log | Synthetic evidence for maintenance-window scenario (LocateSup) |
| crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/maintenance-window/evidence/client-maintenance-window/current/ServiceWindowManager.log | Synthetic evidence for maintenance-window scenario (defer) |
| crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/maintenance-window/evidence/client-updates/current/ScanAgent.log | Synthetic evidence for maintenance-window scenario (Scan/Evaluate) |
| crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/no-sup/expected.json | Expected output for no-sup scenario |
| crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/no-sup/manifest.json | Manifest for no-sup scenario |
| crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/no-sup/evidence/client-updates/current/ScanAgent.log | Synthetic evidence for no-sup scenario (Scan/Evaluate only) |
| crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/reboot-pending/expected.json | Expected output for reboot-pending scenario |
| crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/reboot-pending/manifest.json | Manifest for reboot-pending scenario |
| crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/reboot-pending/evidence/client-location-services-shared/current/LocationServices.log | Synthetic evidence for reboot-pending scenario (LocateSup) |
| crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/reboot-pending/evidence/client-reboot/current/RebootCoordinator.log | Synthetic evidence for reboot-pending scenario (Reboot pending) |
| crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/reboot-pending/evidence/client-updates/current/ScanAgent.log | Synthetic evidence for reboot-pending scenario (Scan/Evaluate) |
| crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/reboot-pending/evidence/client-updates/current/UpdatesDeployment.log | Synthetic evidence for reboot-pending scenario (Download/MW/Install) |
| crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/reporting-failure/expected.json | Expected output for reporting-failure scenario |
| crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/reporting-failure/manifest.json | Manifest for reporting-failure scenario |
| crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/reporting-failure/evidence/client-location-services-shared/current/LocationServices.log | Synthetic evidence for reporting-failure scenario (LocateSup) |
| crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/reporting-failure/evidence/client-policy-state/current/StateMessage.log | Synthetic evidence for reporting-failure scenario (Report failure) |
| crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/reporting-failure/evidence/client-updates/current/ScanAgent.log | Synthetic evidence for reporting-failure scenario (Scan/Evaluate) |
| crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/reporting-failure/evidence/client-updates/current/UpdatesDeployment.log | Synthetic evidence for reporting-failure scenario (Download/MW/Install/Reboot) |
| crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/rotation-boundary/expected.json | Expected output for rotation-boundary scenario |
| crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/rotation-boundary/manifest.json | Manifest for rotation-boundary scenario |
| crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/rotation-boundary/evidence/client-updates/current/ScanAgent.log | Synthetic evidence for rotation-boundary scenario (partial current fragment) |
| crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/rotation-boundary/evidence/client-updates/lo/ScanAgent.lo_ | Synthetic evidence for rotation-boundary scenario (partial archived fragment) |
| crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/same-minute-separate/expected.json | Expected output for same-minute-separate scenario |
| crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/same-minute-separate/manifest.json | Manifest for same-minute-separate scenario |
| crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/same-minute-separate/evidence/client-updates/current/UpdatesHandler.log | Synthetic evidence for same-minute-separate scenario (two keys at same instant) |
| crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/scan-failure/expected.json | Expected output for scan-failure scenario |
| crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/scan-failure/manifest.json | Manifest for scan-failure scenario |
| crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/scan-failure/evidence/client-updates/current/ScanAgent.log | Synthetic evidence for scan-failure scenario (Scan terminal failure) |
| crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/success/expected.json | Expected output for success scenario |
| crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/success/manifest.json | Manifest for success scenario |
| crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/success/evidence/client-content/current/DataTransferService.log | Synthetic evidence for success scenario (Download) |
| crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/success/evidence/client-location-services-shared/current/LocationServices.log | Synthetic evidence for success scenario (LocateSup) |
| crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/success/evidence/client-maintenance-window/current/ServiceWindowManager.log | Synthetic evidence for success scenario (MW open) |
| crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/success/evidence/client-policy-state/current/StateMessage.log | Synthetic evidence for success scenario (Report) |
| crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/success/evidence/client-reboot/current/RebootCoordinator.log | Synthetic evidence for success scenario (Reboot complete) |
| crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/success/evidence/client-updates/current/ScanAgent.log | Synthetic evidence for success scenario (Scan/Evaluate) |
| crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/success/evidence/client-updates/current/UpdatesHandler.log | Synthetic evidence for success scenario (Install) |
| crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/supplemental-conflict/expected.json | Expected output for supplemental-conflict scenario |
| crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/supplemental-conflict/manifest.json | Manifest for supplemental-conflict scenario |
| crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/supplemental-conflict/evidence/client-updates/current/UpdatesHandler.log | Synthetic evidence for supplemental-conflict scenario (client install success) |
| crates/cmtraceopen-parser/tests/fixtures/sccm/client/updates/supplemental-conflict/evidence/client-windows-update-supplemental/current/CBS.log | Synthetic evidence for supplemental-conflict scenario (unkeyed CBS symptom) |
Scope
LocationServices.logevidence before emitting any correlation-ready SUP counterpart factFixture matrix
17 synthetic scenarios: success, no-sup, scan-failure, evaluation-failure, content-failure, maintenance-window, reboot-pending, install-failure, reporting-failure, supplemental-conflict, incomplete, rotation-boundary, capped, access-denied, malformed, invalid-offset, and same-minute-separate.
Corpus identity: 51 manifest artifacts, 43 physical evidence files, 23,142 bytes, 61 lines, 57 complete CCM records, 2 partial rotation fragments, 1 exact capped prefix, and no orphan evidence.
Dependencies
Production implementation remains blocked on reviewed #318 shared contracts and #319 client intake interfaces. This PR intentionally defines proposed preparation labels only and makes no native Windows acceptance claim.
Verification
cargo test --locked -p cmtraceopen-parser --test sccm_client_updates_fixture_contract -- --nocapture— 3 passedcargo test --locked -p cmtraceopen-parser --quiet— 634 passedcargo clippy --locked -p cmtraceopen-parser --all-targets -- -D warnings— passedcargo check --locked -p cmtraceopen-parser --target wasm32-unknown-unknown— passednpx tsc --noEmit— passedrustfmt +1.88.0 --edition 2021 --check crates/cmtraceopen-parser/tests/sccm_client_updates_fixture_contract.rs— passedgit diff --check origin/codex/parser-family-skeleton..HEAD— passedcargo fmt --all -- --check— still reports unrelated pre-existing formatting drift outside this issue-owned slice; the owned Rust file passes Rust 1.88 formattingRestack evidence
Original
c342e946..c4b8656and rebasedc8119325..299f9a1have stable patch IDda5fdbedc3306d89ec64fe8956d62323163b6653;git range-diffreports=.Review state
Local CodeRabbit was invoked for the exact committed range but returned a recoverable 20-minute CLI seat/rate limit, so it is not counted as approval. A GitHub CodeRabbit review will be requested against the pushed head.
Refs #323
Summary by CodeRabbit