feat(sccm): analyze client deployment transactions - #407
Closed
adamgell wants to merge 16 commits into
Closed
Conversation
Each case was proven by mutating the guard it covers; all eight mutations fail the matching test and were reverted. Refs #322
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds a completeness-aware SCCM evidence model and physical-line normalization. Adds a client deployment reducer that validates evidence, builds transactions, resolves outcomes, emits findings and handoffs, and exposes public analysis contracts with comprehensive tests. ChangesSCCM client deployment
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant SccmNormalizedBundle
participant analyze_client_deployment
participant DeploymentFactParser
participant TransactionBuilder
participant SccmDeploymentAnalysis
SccmNormalizedBundle->>analyze_client_deployment: normalized client artifacts and evidence
analyze_client_deployment->>DeploymentFactParser: validate and classify records
DeploymentFactParser->>TransactionBuilder: admitted deployment facts
TransactionBuilder->>SccmDeploymentAnalysis: transactions and outcome evidence
analyze_client_deployment-->>SccmDeploymentAnalysis: findings, requests, and correlation handoff
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
Owner
Author
|
@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 23 minutes. |
github-actions
Bot
added
enhancement
18 tasks
adamgell
commented
Aug 1, 2026
adamgell
left a comment
adamgell
left a comment
Owner
Author
There was a problem hiding this comment.
Review: PR #407, SCCM #322 client deployment reducer\n\nReviewed at exact head b4b2f54 in a detached worktree at\n/Users/Adam.Gell/repo/cmtraceopen/.worktrees/review-407-exact, verified\ngit status --porcelain empty at start and at end. All probe files were removed\nafter use; nothing was committed or pushed.\n\nVerdict: BLOCK. Four defects, each reproduced with an executable probe. Three of\nthem are cases where a guard that exists elsewhere in the same file was not\napplied at a sibling call site.\n\n---\n\n## 1. Battery\n\nAll counts reproduce exactly as declared in the PR body.\n\n\ncargo test --locked -p cmtraceopen-parser --test sccm_client_deployment\n test result: ok. 22 passed; 0 failed\n\ncargo test --locked -p cmtraceopen-parser --test sccm_client_deployment_fixture_contract\n test result: ok. 8 passed; 0 failed\n\ncargo test --locked -p cmtraceopen-parser --test sccm_spine_contract\n test result: ok. 137 passed; 0 failed\n\ncargo test --locked -p cmtraceopen-parser\n 17 test binaries, TOTAL PASSED: 947, 0 failed\n\ncargo test --locked --workspace\n WORKSPACE_EXIT=0, TOTAL PASSED: 1701, 0 failed\n\ncargo clippy --locked -p cmtraceopen-parser --all-targets -- -D warnings\n Finished, CLIPPY_EXIT=0\n\ncargo check --locked -p cmtraceopen-parser --target wasm32-unknown-unknown\n Finished, WASM_EXIT=0\n\ngit diff --check 26116027 HEAD\n DIFF_CHECK_CLEAN\n\nrustfmt --edition 2021 --check, per changed file (no bare cargo fmt was run)\n OK on all 11 changed .rs files\n\n\nMutation proofs for RED pair 3: four of the eight declared mutations were\nre-applied and reverted. Each failed exactly its own test and nothing else.\n\n\nM1 drop the client-role filter on the artifact map\n FAILED. 21 passed; 1 failed -> an_artifact_from_another_role_never_decides_a_client_source\n\nM3 assume every chain is ordered (chain_has_usable_order -> true)\n FAILED. 21 passed; 1 failed -> an_unorderable_terminal_record_downgrades_to_a_low_confidence_symptom\n\nM4 a duplicated exact-token label picks the first value\n FAILED. 21 passed; 1 failed -> a_duplicated_key_label_fails_closed_for_the_whole_record\n\nM8 match an event phrase anywhere in the clause (== -> contains)\n FAILED. 21 passed; 1 failed -> a_label_embedded_in_a_longer_phrase_is_not_a_requirements_outcome\n\n\nThe mutation-proof claim for pair 3 holds.\n\n---\n\n## 2. PR claims verified independently\n\nCounterpart-ready facts appear in exactly six scenarios and nowhere else, and\nall four correlation handoff flags are false in all twelve:\n\n\nPROBE I: bits-transfer-failure counterpartFacts=1\nPROBE I: cache-failure counterpartFacts=1\nPROBE I: dependency-failure counterpartFacts=0\nPROBE I: detection-false-negative counterpartFacts=1\nPROBE I: dp-content-missing counterpartFacts=1\nPROBE I: enforcement-exit counterpartFacts=1\nPROBE I: incomplete counterpartFacts=0\nPROBE I: location-missing counterpartFacts=0\nPROBE I: not-targeted counterpartFacts=0\nPROBE I: requirements-failure counterpartFacts=0\nPROBE I: rotation-boundary counterpartFacts=0\nPROBE I: success counterpartFacts=1\nPROBE I: scenarios with counterpart facts = 6\n(assertion: no scenario has performed, time_only_eligible,\n topology_compatibility_evaluated, or server_cause_claimed set; passed for all 12)\n\n\nThe success policy-coverage claim holds. I first queried the wrong row: the\nsuccess scenario carries client-policy-state: captured and\nclient-policy-agent: absent. The PolicyAgent row is the #321 surface, and it\nis absent, so the independence claim is correct as written.\n\nField-by-field spot check of three scenarios against their expected.json,\nnormalizing only the declared shape differences (entryId omitted, lineStart\nand lineEnd versus startLine and endLine, and absent optionals emitted as\nexplicit null):\n\n\nsuccess transactions=True observations=True\nrotation-boundary transactions=True observations=True\nbits-transfer-failure transactions=True observations=True\n\n\nCoverage rows also match; the only delta is that the reducer emits an empty\nartifactIds: [] where the corpus omits the field, which is equivalent.\n\nnormalize_physical_lines was probed directly and does emit only lines that no\ncomplete logical record covers:\n\n\nPROBE G: content = one 3-line CCM record followed by one stray line\nPROBE G: fragments=[(Some(4), \"[sccm-public-message-v1] stray tail line\")]\n(assert fragments.len() == 1, line_start == Some(4))\n\n\nThe emitted snapshot carries no timestamp and no component, as claimed. The\nclaim that a fragment can therefore never be ordered or promoted is false; see\ndefect 1.\n\n---\n\n## 3. Defects\n\n### BLOCK-1 (critical): a physical-line fragment is admitted as a first-class deployment fact\n\nadmitted_source gates on artifact.coverage == SccmCoverageState::Captured,\nwhich is an artifact-level property. Nothing on the evidence itself records\nwhether it came from a complete logical record, and\nSccmRawEvidenceSnapshot::from_physical_line copies role from the artifact.\nnormalize_physical_lines is called on every artifact including Captured ones.\nConsequently a bare, unframed physical line in a Captured catalog artifact\npasses the role check, the coverage check, valid_reference, the version\nprofile gate, and the catalog identity check, and is parsed as a deployment\nfact.\n\nProbe: line 1 is a complete CCM targeted record in AppIntentEval.log; line 2\nis a bare line reading Requirements satisfied assignmentId=... ciId=....\n\n\nPROBE B: fragment line Some(2) message=\"[sccm-public-message-v1] Requirements satisfied assignmentId=1000...a1 ciId=2000...a2\"\nPROBE B: phase=LocateContent lastSuccessfulPhase=Some(Requirements) observations=0\nPROBE B: evidence=[SccmEvidenceRef { artifact_id: \"client-appintenteval\", entry_id: \"client-appintenteval:1-2\", line_start: Some(1), line_end: Some(2) }]\n\n\nThe fragment reached a transaction, participated in the line-order comparison in\ncompare_fact_order (same artifact, so ordering is by line and the missing\ntimestamp is irrelevant), promoted lastSuccessfulPhase from Intent to\nRequirements, and was folded into an evidence span that presents lines 1 through\n2 as if both were records. It also did not appear in sourceLocalObservations,\nso the fragment was consumed rather than surfaced.\n\nThe same path reaches a terminal verdict:\n\n\nPROBE B2: state=Failed classification=ConfirmedFailure confidence=High\nPROBE B2: findings=[\"deployment-requirements-terminal\"]\n\n\nA single unframed text line asserting terminal=true manufactures a\nHigh-confidence confirmed failure.\n\nThe shipped corpus does not catch this. Scanning all twelve scenarios for\nCaptured artifacts that yield physical-line fragments:\n\n\nPROBE H: enforcement-exit/deployment-enforcement-exit-supplemental-current captured=true fragments=1\nPROBE H: incomplete/deployment-incomplete-enforce-capped captured=false fragments=1\nPROBE H: rotation-boundary/deployment-rotation-boundary-content-current captured=false fragments=1\nPROBE H: rotation-boundary/deployment-rotation-boundary-content-lo captured=false fragments=1\n\n\nThe only Captured artifact with a fragment is the supplemental one, and it is\nexcluded twice over: its basename InstallerSupplemental.log is not in the\ndeployment_source table, and its sourceVersion is null so the version gate\nrejects it. The corpus's safety here comes from the artifact's name, not from\nany property of the evidence. Real CCM logs routinely contain non-conforming\nlines inside catalog-named files.\n\nClass check: admitted_source is the only completeness gate in the reducer, and\nevery one of the six DeploymentSourceKind branches is reached through it, so\nall eight phases are affected, not only Requirements.\n\nFix direction: record completeness on the evidence, not on the artifact. Either\ncarry an explicit per-evidence flag distinguishing a scanned logical record from\na physical-line residue, or give from_physical_line a distinguishable evidence\nidentity that admitted_source rejects. The artifact-level coverage state\ncannot stand in for record completeness, because one artifact can contain both.\n\n### BLOCK-2 (high): the counterpart-ready fact bypasses the key ambiguity guard\n\nbuild_key routes every key field through unique_value, so two disagreeing\nrecords collapse to None and the profile kind degrades to assignmentCi. That\nis correct. counterpart_ready_fact does not use unique_value. It calls\nfirst_fact(facts, ContentLocated), which returns the first match in a slice\nsorted by compare_references, that is the lowest (artifact_id, line_start,\nline_end, entry_id), and then reads that single fact's fields directly.\n\nProbe: two complete content located records in one CAS artifact that disagree\non packageId, contentId, contentVersion, distributionPointHostHandle,\nand requestId.\n\n\n(assertions passed) key.content_id == None, key.request_id == None,\n key.package_id == None,\n key.key_profile_kind == AssignmentCi\nPROBE A: key contentId=None/requestId=None but counterpart fact publishes\n contentId=30000000-0000-0000-0000-0000000000a3\n requestId=40000000-0000-0000-0000-0000000000a4\n packageId=LAB00021 dp=safe:dp:lab-dp-02\n(assertion passed) correlation_handoff.emitted_counterpart_ready_fact == true\n\n\nThe transaction publicly declines to state the content topology, and the only\ncross-side handoff in the analysis publishes a definite one anyway.\n\nAcross artifacts the selection is decided by artifact id spelling, not by time:\n\n\nPROBE A2: chose contentId=30000000-...-c3 requestId=40000000-...-d4\n normalizedUtc=2026-07-30T05:00:09Z\n (alphabetically-first artifact, chronologically LAST record)\n\n\nclient-aaa-cas at 05:00:09 beat client-zzz-cas at 05:00:02. Renaming an\nartifact changes which content request is handed to #333, and the fact's own\ntimestampProvenance then attests to the later record.\n\nClass check: this is precisely the mutation the PR body lists as already proven,\n"letting an ambiguous key pick the lowest". That mutation targeted\nunique_value. counterpart_ready_fact is the sibling call site in the same\nfunction family that was never brought under the guard. I checked the other\nconsumers of first_fact: resolve_outcome uses it for chain construction,\nwhere a disagreement is later caught by unique_value in build_key or by\nchain_has_usable_order, so counterpart_ready_fact is the one unguarded exit.\n\nFix direction: derive the counterpart fact from the transaction key rather than\nfrom a single fact. Emit it only when key.key_profile_kind is\nAssignmentCiContentTopology and every one of the five required fields is\nSome, and take the values from the key. The evidence reference and the\ntimestamp provenance should then come from the fact that actually carries those\nexact values, or the fact should not be emitted at all.\n\n### BLOCK-3 (high): the duplicate-label guard filters candidates before it counts them\n\nIn field_value, the left-boundary filter requiring index 0 or a preceding\nASCII whitespace byte is applied to the match_indices iterator, and only then\nis the duplicate test if matches.next().is_some() { return None; } evaluated.\nA second, conflicting occurrence preceded by punctuation is removed by the\nfilter before the guard can see it, so the guard does not fire and the first\nvalue is returned.\n\nProbe, on the enforcement terminal path:\n\n\nPROBE C: whitespace duplicate `terminal=false` -> (InsufficientEvidence, InsufficientEvidence, Low, Enforce)\nPROBE C: paren duplicate `(terminal=false)` -> (Failed, ConfirmedFailure, High, Enforce)\n(assertion passed) paren case confidence == High\n\n\nThe same record text, with the conflicting marker wrapped in parentheses,\nconverts a correctly withheld verdict into a High-confidence confirmed failure.\n\nSame shape on the field that is carried in the public key:\n\n\nPROBE C2: whitespace duplicate `exitCode=1603` -> (InsufficientEvidence, Low, exit_code None)\nPROBE C2: paren duplicate `(exitCode=1603)` -> (InsufficientEvidence, Low, exit_code Some(\"0\"))\n\n\nClass check: field_value is the single accessor for every key and gating field\nin the reducer. The affected set is assignmentId, ciId, packageId,\ncontentId, contentVersion, distributionPointHostHandle, requestId,\nbitsJobId, productCode, exitCode, state, terminal, errorCode,\ndetected, responseState, requirementId, and dependencyCiId. Every phase\nbinding and every terminality gate inherits the gap, so this is not one field.\n\nFix direction: count first, filter second. Collect all occurrences of the\nkey= marker regardless of the preceding byte, and fail closed if more than one\nis found. Then apply the boundary test to the single survivor to reject\nembedded tails such as NotRequestId=. Alternatively keep the filter but widen\nthe accepted left boundary to any non-alphanumeric character, so a bracketed or\nparenthesised occurrence is counted as the ambiguity it is rather than silently\ndiscarded.\n\n### BLOCK-4 (medium): one chronology finding lets the lowest assignment id speak for the group\n\nbuild_findings groups seeds by finding_id and then uses seeds.first() as\nthe authority for class, phase, confidence, last_successful_phase,\ncoverage_gap_group, and request_phase. Every other finding id is\nphase-homogeneous by construction, but FINDING_CHRONOLOGY_UNCERTAIN is emitted\nfrom conclude, which is called at eight different phases with eight different\nlast_successful_phase values. Seed order follows the by_assignment BTreeMap,\nso the assignment GUID that sorts lowest decides what the merged finding says.\n\nProbe: assignment ...a1 unorderable at Requirements, assignment ...b1\nunorderable at Enforce.\n\n\nPROBE D: transactions=[\n (\"deployment:assignment:10000000-...-a1\", Requirements, InsufficientEvidence, Low, Some(Intent)),\n (\"deployment:assignment:10000000-...-b1\", Enforce, InsufficientEvidence, Low, Some(Cache))]\nPROBE D: findings=[\n (\"deployment-chronology-uncertain\", Requirements, Some(Intent), 6,\n [SccmArtifactRequest { logical_id: \"appIntentEval\", role: Client,\n reason: \"Collect the complete AppIntentEval.log file.\" }])]\n\n\nOne finding reports deploymentPhase: Requirements and\nlastSuccessfulPhase: Intent, cites six evidence references drawn from both\ntransactions, and requests only AppIntentEval.log. The Enforce transaction's\nactual need, AppEnforce.log, is erased. Swapping the two assignment GUIDs\nflips the reported phase and the requested artifact without changing any\nevidence. This is a coverage gap erased by a captured sibling.\n\nFix direction: group chronology seeds by (finding_id, phase) rather than by\nfinding_id alone, or accumulate the union of request_phase values and emit\none SccmArtifactRequest per distinct phase. Merging is only sound where the\nmerged seeds agree on the fields the first seed currently dictates.\n\n---\n\n## 4. Risk areas that passed\n\nVector-order authority. Forward versus fully reversed artifacts and evidence\nproduced byte-identical serialized analyses on the synthetic adversarial\nbundles, including one carrying a cross-role duplicate artifact id:\n\n\nPROBE E: reordering stable on both synthetic bundles\n\n\nThe shipped reordering_the_bundle_never_changes_the_analysis covers all twelve\ncorpus scenarios. bundle_identity_collides correctly fails closed to\ncoverage-only output on duplicate client artifact ids, duplicate evidence ids,\nand duplicate reference tuples, and it is correctly scoped to client-role\nartifacts so a same-named artifact in another role cannot be elected.\ncombine_coverage, coverage_rows, extraction_profile, merged_evidence,\nand merge_reference_spans are all order-independent by construction. The\nfirst_fact selection is deterministic under reordering, though BLOCK-2 shows\ndeterminism is not by itself correctness.\n\nRaw-substring binding. SccmRawEvidenceSnapshot::from_record sets\nmessage: entry.message, the parsed CCM body, so the\n<time=... date=... component=... context=...> trailer is never in scope for\nfield_value. deployment_event_payload further requires the\n[sccm-public-message-v1] projection prefix. event_phrase is anchored at the\nclause start, terminated at the first =-bearing token, and only documented\nqualifiers are skipped, so Base requirements satisfied does not bind\nrequirements satisfied. Mutation M8 confirms the anchoring is load-bearing.\nNo phase, state, or terminal binding uses a raw contains over a whole line.\n\nConfidence that can only go up. Adding an artifact that carries a complete and\nvalid content chain but is Absent, Partial, Capped, or version-unprofiled never\nmoved the transaction:\n\n\nPROBE F [absent]: phase LocateContent -> LocateContent, confidence Low -> Low\nPROBE F [partial]: phase LocateContent -> LocateContent, confidence Low -> Low\nPROBE F [capped]: phase LocateContent -> LocateContent, confidence Low -> Low\nPROBE F [unprofiled]: phase LocateContent -> LocateContent, confidence Low -> Low\n\n\nUnorderable evidence caps rather than promotes: conclude downgrades any\nunusable chain to Low symptom regardless of the outcome it was called with, and\nmutation M3 confirms that guard is load-bearing.\n\n---\n\n## 5. Shared-spine compatibility\n\nNine lanes stack on this spine. Each shared change was assessed.\n\nSccmCoverageState::Partial inserted as the second variant in models.rs. The\nenum derives Debug, Clone, PartialEq, Serialize, Deserialize with\nrename_all = \"camelCase\". It does not derive Ord or PartialOrd, and no\ncall site casts it with as u8, so inserting a variant in the middle cannot\nshift any comparison. Serde is name-based in both directions, so existing\nserialized values continue to round-trip and \"partial\" is purely additive.\n\ncoverage_state_order in findings.rs and coverage_sort_key in\nserver/windows/intake.rs. Both were renumbered or extended to admit Partial.\ncoverage_state_order shifts Absent through ParseFailed each up by one, which\npreserves the relative order of all seven pre-existing variants; its only use is\nthe comparator at findings.rs:2198, so no merged lane's sort changes.\ncoverage_sort_key returns a &'static str used as a sort key at\nintake.rs:218, and \"partial\" sorts between \"parseFailed\" and \"skipped\",\nwhich again cannot reorder any existing pair. No existing code path produces\nPartial, so no merged lane can observe the new variant at all until it opts in.\n\nNo frontend mirror of SccmCoverageState exists. The only TypeScript union\ncarrying a parseFailed member is the ESP types.ts coverage type, which is a\ndifferent enum with different members, so there is no TS surface to update. The\nworkspace build and all 1701 workspace tests pass.\n\nnormalize_physical_lines in ingest.rs and\nSccmRawEvidenceSnapshot::from_physical_line in evidence.rs are new functions\nwith no pre-existing callers, so they are additive. Note that making\nnormalize_physical_lines public without a corresponding per-evidence\ncompleteness marker is what makes BLOCK-1 a spine-level rather than a lane-level\nproblem: any of the nine stacked lanes that adopts it inherits the same\npromotion path.\n\nThe StateMessage catalog entry in catalog.rs is additive. It maps to\nlogical_name: \"stateMessage\", role Client, family ClientPolicy. It does not\nshadow an existing basename, and the sccm_spine_contract suite still passes at\n137.\n\nThe PR discloses that the Partial variant and normalize_physical_lines are\nidentical in intent to changes on the #320 health branch and that whichever\nmerges second needs a conflict resolution. That disclosure is accurate and\nappreciated.\n\n---\n\n## 6. Judgment on the declared deliberate differences\n\nReporting extractionProfile.keyKinds as observed rather than as the profile's\ndeclared ten-kind capability list is the right call. A capability list is not\nevidence, and four scenarios declaring capability they did not exercise is a\ncorpus overstatement, correctly surfaced rather than matched.\n\nReporting validatedArtifactFamilies as only those families that yielded an\nadmitted record is not merely acceptable, it is strictly better than the corpus.\nThe rotation-boundary case, where the corpus declares client-content\nvalidated although neither fragment ever formed a record, is exactly the\n"coverage claim that survives when its source does not" failure this program is\ntrying to eliminate. I confirmed the reducer emits\n[\"client-app-intent\"] where the corpus declares\n[\"client-app-intent\", \"client-content\"]. This should be treated as a corpus\ndefect to be filed, not as a reducer deviation to be tolerated indefinitely.\n\nDerived observation ids, non-exhaustive corpus findings, and the two request\nshapes are all correctly reasoned and correctly disclosed. The two request\nshapes in particular are forced by the merged spine's validator, which accepts\nonly declared catalog logical ids, and inventing a third shape here would be\nworse.\n\nOne undisclosed difference: every one of the twelve expected.json files\ncarries extractionProfile.selectionState: \"selected\", and\nSccmDeploymentExtractionProfile has no such field. grep for\nselectionState or selection_state across\ncrates/cmtraceopen-parser/src/ returns nothing. This is minor, but it belongs\nin the deliberate-differences list, because "12 of 12 scenarios reproduce\nexactly" is not quite true of the extraction profile object.\n\n---\n\n## 7. Minor observations, not blocking\n\nTwo disagreeing coverage precedence orders live in the same file.\ncombine_coverage ranks Capped above Partial in its explanatory precedence\nlist, while coverage_order ranks Partial at 1 and Capped at 4. A group mixing\na Capped and a Partial artifact therefore reports Capped, and resolve_outcome\nthen selects REASON_LOCATION_ABSENT instead of REASON_LOCATION_ROTATION,\nlosing the rotation explanation that the Partial artifact justified. Worth\nreconciling to a single precedence.\n\nbuild_findings pushes a group id such as client-app-intent into\nSccmFindingCoverageGap.artifact_id when no incomplete artifact is available to\nblame. Every other producer of that field fills it with a real artifact id. A\nconsumer joining coverageGaps[].artifactId against the artifact inventory will\nsilently miss. The intent is sound and well commented; the field choice is\nlikely to surprise.\n\n---\n\n## 8. Summary\n\nBlocking: BLOCK-1 fragment promotion, BLOCK-2 counterpart fact bypassing the key\nambiguity guard, BLOCK-3 punctuation-adjacent duplicate labels defeating the\nexact-token guard, BLOCK-4 heterogeneous chronology findings decided by the\nlowest assignment id.\n\nBLOCK-1, BLOCK-2, and BLOCK-3 are all cases where the correct guard already\nexists elsewhere in this file and was not extended to a sibling path. BLOCK-2 in\nparticular is the untouched sibling of a mutation the PR body lists as proven.\nI would ask that each fix be accompanied by a check of the remaining call sites\nof the same shape rather than a targeted repair of the reported instance.\n\nThe reducer's conservative architecture is otherwise sound and the verification\ndiscipline in the PR is genuinely strong: every count reproduced, the mutation\nproofs hold, reordering determinism holds, and confidence monotonicity holds.\nThe four defects are all in the seams between guards rather than in the guards\nthemselves.\n\nReviewed at exact head b4b2f54. Worktree\n/Users/Adam.Gell/repo/cmtraceopen/.worktrees/review-407-exact was verified\nclean before and after; no commits, pushes, or bare cargo fmt were run."}],"stop_reason":"end_turn","stop_sequence":null,"stop_details":null,"usage":{"input_tokens":2,"cache_creation_input_tokens":144163,"cache_read_input_tokens":18506,"output_tokens":9116,"server_tool_use":{"web_search_requests":0,"web_fetch_requests":0},"service_tier":"standard","cache_creation":{"ephemeral_1h_input_tokens":0,"ephemeral_5m_input_tokens":144163},"inference_geo":"not_available","iterations":[{"input_tokens":2,"output_tokens":9116,"cache_read_input_tokens":18506,"cache_creation_input_tokens":144163,"cache_creation":{"ephemeral_5m_input_tokens":144163,"ephemeral_1h_input_tokens":0},"type":"message"}],"speed":"standard"},"diagnostics":{"cache_miss_reason":{"type":"messages_changed","cache_missed_input_tokens":120403}}},"requestId":"req_011CdbD1WBtJYNGGbgaWHA1e","attributionAgent":"general-purpose","type":"assistant","uuid":"e1b180b9-0ea6-4fc9-821c-d153e340bdaa","timestamp":"2026-08-01T02:06:03.201Z","effort":"xhigh","userType":"external","entrypoint":"claude-desktop","cwd":"/Users/Adam.Gell/repo/cmtraceopen","sessionId":"c5e32ddb-04e7-4474-a318-c84dd6ce5688","version":"2.1.219","gitBranch":"main"}
Owner
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
crates/cmtraceopen-parser/src/sccm/client/deployment.rs (2)
637-648: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCentralize the
SccmCoverageStateordering table. Three modules now hold the same variant ranking. The shared root cause is the absence of one ordering helper next to the enum, so every new coverage state requires three coordinated edits and can drift.
crates/cmtraceopen-parser/src/sccm/client/deployment.rs#L637-L648: deletecoverage_orderand call the shared helper.crates/cmtraceopen-parser/src/sccm/findings.rs#L2262-L2273: replacecoverage_state_orderwith the shared helper, or promote this function to the shared location besideSccmCoverageStateincrates/cmtraceopen-parser/src/sccm/models.rs.crates/cmtraceopen-parser/src/sccm/server/windows/intake.rs#L1076-L1087: derivecoverage_sort_keyfrom the shared helper so the rank and the serialized name stay in one place.🤖 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/deployment.rs` around lines 637 - 648, Centralize the SccmCoverageState ranking beside the enum in sccm/models.rs and expose one shared ordering helper. In crates/cmtraceopen-parser/src/sccm/client/deployment.rs:637-648, remove coverage_order and use the shared helper; in crates/cmtraceopen-parser/src/sccm/findings.rs:2262-2273, replace coverage_state_order with it; and in crates/cmtraceopen-parser/src/sccm/server/windows/intake.rs:1076-1087, derive coverage_sort_key from the shared helper while preserving the serialized coverage name.
1935-2002: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the unmatched finding id explicit instead of defaulting to report text.
The final
_ =>arm returns the report coverage-gap title and summary. If a later change adds a finding id and forgets the text entry, the reducer emits a wrong user-facing title with no compile error. Match"deployment-report-coverage-gap"explicitly and make the remaining arm unreachable, or returnOptionand let the caller decide.♻️ Proposed refactor
- _ => ( + "deployment-report-coverage-gap" => ( "Client deployment state report evidence is incomplete", "No complete client state report was available for this content key.", ), + other => unreachable!("unmapped deployment finding id {other}"), } }🤖 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/deployment.rs` around lines 1935 - 2002, Update finding_text so "deployment-report-coverage-gap" is matched explicitly with the existing report coverage-gap title and summary. Replace the current catch-all fallback with an unreachable remaining arm, or change the function to return Option and handle unknown finding IDs at the caller, ensuring unmatched IDs cannot silently produce report text.crates/cmtraceopen-parser/tests/sccm_client_deployment.rs (1)
1358-1429: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winState why 4 scenarios skip the
keyKindsassertion and 5 skip the family assertion.
OBSERVED_KEY_KIND_SCENARIOSomitsdependency-failure,location-missing,not-targeted, andrequirements-failure.OBSERVED_FAMILY_SCENARIOSadditionally omitsrotation-boundary. Only therotation-boundaryomission carries a documented reason. For the other scenarios the declared corpus values forkeyKindsandvalidatedArtifactFamiliesare never compared, so a regression there passes silently.Either assert those scenarios as well, or record the corpus reason for each exclusion in the doc comments.
🤖 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_deployment.rs` around lines 1358 - 1429, Document the intentional exclusions from OBSERVED_KEY_KIND_SCENARIOS and OBSERVED_FAMILY_SCENARIOS, explaining the corpus reason for dependency-failure, location-missing, not-targeted, requirements-failure, and rotation-boundary; alternatively include those scenarios in the corresponding assertions if their declared values are observation-derived. Ensure every skipped keyKinds or validatedArtifactFamilies comparison has an explicit rationale.crates/cmtraceopen-parser/src/sccm/ingest.rs (1)
20-40: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid the per-line scan over all record spans.
For each physical line the closure scans every record span. Cost is O(lines x records), so a large CCM log with many records makes ingest quadratic.
scan_logical_recordsalso runs twice per artifact when a caller needs both logical records and fragments, as the tests do atcrates/cmtraceopen-parser/tests/sccm_client_deployment.rslines 115-116.The scanner emits records in increasing line order, so a single forward walk removes the inner scan.
⚡ Proposed fix: advance one cursor through the sorted spans
pub fn normalize_physical_lines(artifact: &SccmArtifact, content: &str) -> Vec<SccmEvidence> { let covered = scan_logical_records(content, &artifact.display_name) .into_iter() .map(|record| (record.line_start, record.line_end)) .collect::<Vec<_>>(); + let mut next_span = 0usize; content .lines() .enumerate() .filter_map(|(index, line)| { let line_number = u32::try_from(index + 1).ok()?; - let is_covered = covered - .iter() - .any(|(start, end)| (*start..=*end).contains(&line_number)); + while next_span < covered.len() && covered[next_span].1 < line_number { + next_span += 1; + } + let is_covered = covered + .get(next_span) + .is_some_and(|(start, end)| (*start..=*end).contains(&line_number)); if is_covered || line.trim().is_empty() { return None; } Some(SccmRawEvidenceSnapshot::from_physical_line(artifact, line_number, line).export()) }) .collect() }Consider also a combined entry point that scans once and returns records plus residue, so callers stop scanning the same content twice.
🤖 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/ingest.rs` around lines 20 - 40, Update normalize_physical_lines to consume the sorted spans from scan_logical_records with a single forward cursor, advancing past exhausted spans instead of scanning covered for every physical line; preserve omission of covered and blank lines. Add a combined scanning entry point that performs one scan and returns both logical records and physical-line residue, then update callers such as the SCCM deployment test flow to reuse that result instead of invoking scan_logical_records twice.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/cmtraceopen-parser/src/sccm/client/deployment.rs`:
- Around line 1444-1456: Update earliest_comparable_fact to validate
comparability for every distinct pair of facts, not only pairs involving the
selected earliest fact; return None immediately when any compare_fact_order call
fails, while preserving the existing earliest-selection behavior otherwise. Add
a three-record test in the SCCM deployment tests covering facts from two
artifacts where one pair is incomparable, and assert that no fact is returned.
- Around line 1458-1465: Update the crate’s chrono dependency constraint to
require version 0.4.35 or newer, while preserving the existing serde feature, so
format_normalized_utc can reliably use DateTime::<Utc>::from_timestamp_millis.
---
Nitpick comments:
In `@crates/cmtraceopen-parser/src/sccm/client/deployment.rs`:
- Around line 637-648: Centralize the SccmCoverageState ranking beside the enum
in sccm/models.rs and expose one shared ordering helper. In
crates/cmtraceopen-parser/src/sccm/client/deployment.rs:637-648, remove
coverage_order and use the shared helper; in
crates/cmtraceopen-parser/src/sccm/findings.rs:2262-2273, replace
coverage_state_order with it; and in
crates/cmtraceopen-parser/src/sccm/server/windows/intake.rs:1076-1087, derive
coverage_sort_key from the shared helper while preserving the serialized
coverage name.
- Around line 1935-2002: Update finding_text so "deployment-report-coverage-gap"
is matched explicitly with the existing report coverage-gap title and summary.
Replace the current catch-all fallback with an unreachable remaining arm, or
change the function to return Option and handle unknown finding IDs at the
caller, ensuring unmatched IDs cannot silently produce report text.
In `@crates/cmtraceopen-parser/src/sccm/ingest.rs`:
- Around line 20-40: Update normalize_physical_lines to consume the sorted spans
from scan_logical_records with a single forward cursor, advancing past exhausted
spans instead of scanning covered for every physical line; preserve omission of
covered and blank lines. Add a combined scanning entry point that performs one
scan and returns both logical records and physical-line residue, then update
callers such as the SCCM deployment test flow to reuse that result instead of
invoking scan_logical_records twice.
In `@crates/cmtraceopen-parser/tests/sccm_client_deployment.rs`:
- Around line 1358-1429: Document the intentional exclusions from
OBSERVED_KEY_KIND_SCENARIOS and OBSERVED_FAMILY_SCENARIOS, explaining the corpus
reason for dependency-failure, location-missing, not-targeted,
requirements-failure, and rotation-boundary; alternatively include those
scenarios in the corresponding assertions if their declared values are
observation-derived. Ensure every skipped keyKinds or validatedArtifactFamilies
comparison has an explicit rationale.
🪄 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: e96e8e1c-1c8b-4bf6-8f57-fc7182428bc0
📒 Files selected for processing (11)
crates/cmtraceopen-parser/src/sccm/catalog.rscrates/cmtraceopen-parser/src/sccm/client/deployment.rscrates/cmtraceopen-parser/src/sccm/client/mod.rscrates/cmtraceopen-parser/src/sccm/evidence.rscrates/cmtraceopen-parser/src/sccm/findings.rscrates/cmtraceopen-parser/src/sccm/ingest.rscrates/cmtraceopen-parser/src/sccm/mod.rscrates/cmtraceopen-parser/src/sccm/models.rscrates/cmtraceopen-parser/src/sccm/server/windows/intake.rscrates/cmtraceopen-parser/tests/sccm_client_deployment.rscrates/cmtraceopen-parser/tests/sccm_spine_contract.rs
Comment on lines
+1444
to
+1456
| fn earliest_comparable_fact<'a>(facts: &[&'a DeploymentFact]) -> Option<&'a DeploymentFact> { | ||
| let mut earliest = *facts.first()?; | ||
| for candidate in &facts[1..] { | ||
| match compare_fact_order(earliest, candidate)? { | ||
| Ordering::Greater => earliest = candidate, | ||
| Ordering::Less | Ordering::Equal => {} | ||
| } | ||
| } | ||
| for candidate in facts { | ||
| compare_fact_order(earliest, candidate)?; | ||
| } | ||
| Some(earliest) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Check all candidate pairs, not only pairs that include earliest.
Comparability is not transitive. compare_fact_order orders two facts in the same artifact by line, and two facts in different artifacts only when both are time_comparable. So three candidates can pass this check while one pair stays incomparable:
- fact A: artifact-1, valid offset
- fact B: artifact-1, no offset, later line
- fact C: artifact-2, valid offset
A orders against B by line and against C by time, so the function returns A. B and C never compare. The published citation then comes from a record set that is not fully orderable, which the doc comment and lines 1399-1401 forbid.
🐛 Proposed fix: verify every pair
fn earliest_comparable_fact<'a>(facts: &[&'a DeploymentFact]) -> Option<&'a DeploymentFact> {
- let mut earliest = *facts.first()?;
- for candidate in &facts[1..] {
- match compare_fact_order(earliest, candidate)? {
- Ordering::Greater => earliest = candidate,
- Ordering::Less | Ordering::Equal => {}
- }
- }
- for candidate in facts {
- compare_fact_order(earliest, candidate)?;
- }
- Some(earliest)
+ let mut earliest = *facts.first()?;
+ for (index, left) in facts.iter().enumerate() {
+ for right in &facts[index + 1..] {
+ if compare_fact_order(left, right)? == Ordering::Greater {
+ // ordering is total across the set; the fold below elects the minimum
+ }
+ }
+ }
+ for candidate in &facts[1..] {
+ if compare_fact_order(earliest, candidate)? == Ordering::Greater {
+ earliest = candidate;
+ }
+ }
+ Some(earliest)
}Add a three-record case to crates/cmtraceopen-parser/tests/sccm_client_deployment.rs that pins this refusal.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| fn earliest_comparable_fact<'a>(facts: &[&'a DeploymentFact]) -> Option<&'a DeploymentFact> { | |
| let mut earliest = *facts.first()?; | |
| for candidate in &facts[1..] { | |
| match compare_fact_order(earliest, candidate)? { | |
| Ordering::Greater => earliest = candidate, | |
| Ordering::Less | Ordering::Equal => {} | |
| } | |
| } | |
| for candidate in facts { | |
| compare_fact_order(earliest, candidate)?; | |
| } | |
| Some(earliest) | |
| } | |
| fn earliest_comparable_fact<'a>(facts: &[&'a DeploymentFact]) -> Option<&'a DeploymentFact> { | |
| let mut earliest = *facts.first()?; | |
| for (index, left) in facts.iter().enumerate() { | |
| for right in &facts[index + 1..] { | |
| if compare_fact_order(left, right)? == Ordering::Greater { | |
| // ordering is total across the set; the fold below elects the minimum | |
| } | |
| } | |
| } | |
| for candidate in &facts[1..] { | |
| if compare_fact_order(earliest, candidate)? == Ordering::Greater { | |
| earliest = candidate; | |
| } | |
| } | |
| Some(earliest) | |
| } |
🤖 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/deployment.rs` around lines 1444 -
1456, Update earliest_comparable_fact to validate comparability for every
distinct pair of facts, not only pairs involving the selected earliest fact;
return None immediately when any compare_fact_order call fails, while preserving
the existing earliest-selection behavior otherwise. Add a three-record test in
the SCCM deployment tests covering facts from two artifacts where one pair is
incomparable, and assert that no fact is returned.
Comment on lines
+1458
to
+1465
| fn format_normalized_utc(millis: i64) -> Option<String> { | ||
| let timestamp = chrono::DateTime::<chrono::Utc>::from_timestamp_millis(millis)?; | ||
| Some(if millis.rem_euclid(1_000) == 0 { | ||
| timestamp.format("%Y-%m-%dT%H:%M:%SZ").to_string() | ||
| } else { | ||
| timestamp.format("%Y-%m-%dT%H:%M:%S%.3fZ").to_string() | ||
| }) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Report the declared and locked chrono versions.
set -euo pipefail
fd -H -t f 'Cargo.toml' --exec rg -n -C2 '^\s*chrono' {} \; || true
fd -H -t f 'Cargo.lock' --exec rg -n -A2 '^name = "chrono"' {} \; || trueRepository: adamgell/cmtraceopen
Length of output: 1958
🌐 Web query:
chrono DateTime Utc from_timestamp_millis added version
💡 Result:
The method DateTime::from_timestamp_millis(millis: i64) -> Option<DateTime<Utc>> was added to the chrono crate in version 0.4.35 [1][2]. This method creates a DateTime<Utc> from the number of milliseconds since the UNIX epoch (January 1, 1970, 0:00:00.000 UTC) [3][1]. It returns None if the provided millisecond value is out of the supported range [3][2]. Note that prior to version 0.4.35, similar functionality was available via NaiveDateTime::from_timestamp_millis, which has since been deprecated in favor of DateTime::from_timestamp_millis [1][2].
Citations:
- 1: https://codebrowser.dev/rust/crates/chrono/src/naive/datetime/mod.rs.html
- 2: https://doc.servo.org/chrono/naive/struct.NaiveDateTime.html
- 3: https://docs.rs/chrono/latest/chrono/struct.DateTime.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
repo='adamgell/cmtraceopen'
tree="$(curl -fsSL "https://api.github.com/repos/${repo}/git/trees/HEAD?recursive=1")"
printf '%s\n' "$tree" |
jq -r '.tree[] | select(.path == "Cargo.lock" or (.path | endswith("Cargo.toml"))) | .path' |
while IFS= read -r path; do
printf '\n--- %s ---\n' "$path"
curl -fsSL "https://raw.githubusercontent.com/${repo}/HEAD/${path}" |
rg -n -C2 '^\s*chrono\s*=|^name = "chrono"|^version = ' || true
doneRepository: adamgell/cmtraceopen
Length of output: 1929
🌐 Web query:
site:github.com/adamgell/cmtraceopen Cargo.toml chrono
💡 Result:
To add the chrono crate as a dependency to your Rust project, add it to the [dependencies] section of your Cargo.toml file. You can add the latest version by specifying it directly: [dependencies] chrono = "0.4" Alternatively, you can add it from your terminal using the cargo add command: cargo add chrono The chrono crate is the standard library for date and time handling in Rust, providing features for ISO 8601 parsing, time zone support, and duration arithmetic. For more detailed information, documentation, and feature flags (such as enabling serde support), you can refer to the official crate page on crates.io.
Citations:
- 1: https://github.com/adamgell/cmtraceopen/releases
- 2: https://github.com/adamgell/cmtraceopen/blob/main/CHANGELOG.md
- 3: docs: explain Full vs Lite editions in README (#227) #241
- 4: https://github.com/adamgell/cmtraceopen/releases/tag/v1.2.0
- 5: fix(build): repair root workspace and sync stale Cargo.lock #240
🌐 Web query:
"adamgell/cmtraceopen" "chrono =" Cargo.toml
💡 Result:
In the adamgell/cmtraceopen repository, the chrono crate is a dependency used for handling date and time, specifically for log parsing and timestamp management [1]. It is defined in the Cargo.toml file of the cmtraceopen-parser crate [1]. The configuration entry specifies: chrono = { version = "0.4", features = ["serde"] } [1] This dependency enables chrono with serde support, allowing the application to serialize and deserialize timestamp data efficiently [1]. The cmtraceopen project, an open-source, cross-platform log viewer, uses this for its log parsing capabilities, which support various log formats including CCM, CBS, and others [2][3][4].
Citations:
- 1: 1a9e34b
- 2: https://github.com/adamgell/cmtraceopen
- 3: https://adamgell.com/tools/cmtrace/
- 4: https://github.com/adamgell/cmtraceopen/releases/tag/v1.4.0
🌐 Web query:
"adamgell/cmtraceopen" "name = \"chrono\"" Cargo.lock
💡 Result:
CMTrace Open is an open-source, cross-platform log viewer developed by adamgell [1][2], designed as a modern replacement for Microsoft's CMTrace.exe [1][3]. Regarding the Cargo.lock file and the chrono crate: 1. Relationship to CMTrace Open: The CMTrace Open repository is a Rust project that utilizes a workspace structure [4][5]. The chrono crate is a dependency used within the project, specifically integrated into the cmtraceopen-parser crate (a standalone component of the project) to handle date and time parsing [6][7]. 2. Cargo.lock Context: Recent development in the project (as of July 2026) addressed issues with the root Cargo.lock file being orphaned and stale [4][5][8]. Commits such as 8ad55ab, 25f9acd, and 5b65ee9 detail how the repository's workspace was repaired to ensure the root Cargo.lock correctly reflects the manifest requirements, resolving build and security audit failures [4][5][8][9]. 3. Cargo Behavior: The inclusion of crates like chrono in Cargo.lock is standard Cargo behavior [10][11]. Cargo generates a platform-independent lockfile that tracks all possible dependencies for all targets, which can sometimes result in entries appearing in the lockfile even if specific features or platform-dependent dependencies are not actively being compiled [10][11][12]. This is a known behavior of the Cargo package manager and is not unique to the CMTrace Open project [10][11].
Citations:
- 1: https://github.com/adamgell/cmtraceopen
- 2: https://github.com/adamgell/CMTraceOpen/wiki
- 3: https://github.com/adamgell/cmtraceopen/releases/tag/v1.4.0
- 4: fix(build): repair root workspace and sync stale Cargo.lock #240
- 5: 8ad55ab
- 6: https://crates.io/crates/cmtraceopen-parser
- 7: https://crates.io/crates/chrono
- 8: 25f9acd
- 9: 5b65ee9
- 10: Disabled optional weak dependencies end up in
Cargo.lockrust-lang/cargo#10801 - 11:
Cargo.lockincludes crates that aren't actually dependencies at all rust-lang/cargo#13078 - 12:
default-features=falsebutchronomakessqlx-mysqlpart of theCargo.lockfile transact-rs/sqlx#3538
Set the minimum chrono version to 0.4.35. chrono = { version = "0.4", features = ["serde"] } permits versions that do not provide DateTime::<Utc>::from_timestamp_millis, which can cause compilation failures.
🤖 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/deployment.rs` around lines 1458 -
1465, Update the crate’s chrono dependency constraint to require version 0.4.35
or newer, while preserving the existing serde feature, so format_normalized_utc
can reliably use DateTime::<Utc>::from_timestamp_millis.
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a new SCCM client deployment transaction analyzer (#322) to cmtraceopen-parser, producing conservative, evidence-cited deployment transactions and findings from a normalized client bundle, plus shared-spine enhancements needed to model partial/fragmentary evidence.
Changes:
- Introduces
cmtraceopen_parser::sccm::analyze_client_deployment(&SccmNormalizedBundle) -> SccmDeploymentAnalysiswith an 8-phase deployment workflow and corpus-driven behavior contracts. - Extends the SCCM spine with
SccmCoverageState::Partial,SccmRecordCompleteness, andnormalize_physical_linesto preserve non-record physical fragments without promoting them to facts. - Updates the SCCM source catalog to recognize
StateMessage.logfor client policy/state reporting coverage.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/cmtraceopen-parser/tests/sccm_spine_contract.rs | Updates spine contract tests for new completeness field and StateMessage catalog entry. |
| crates/cmtraceopen-parser/tests/sccm_client_deployment.rs | Adds corpus-driven behavior contract + adversarial tests for the new deployment reducer. |
| crates/cmtraceopen-parser/src/sccm/server/windows/intake.rs | Adds Partial to coverage sort key handling. |
| crates/cmtraceopen-parser/src/sccm/models.rs | Adds SccmCoverageState::Partial, introduces SccmRecordCompleteness, and adds completeness to SccmEvidence. |
| crates/cmtraceopen-parser/src/sccm/mod.rs | Exposes new sccm::client module and re-exports its public API. |
| crates/cmtraceopen-parser/src/sccm/ingest.rs | Adds normalize_physical_lines to emit residue physical lines not covered by logical records. |
| crates/cmtraceopen-parser/src/sccm/findings.rs | Updates coverage ordering to include Partial. |
| crates/cmtraceopen-parser/src/sccm/evidence.rs | Threads record completeness through evidence export and adds physical-line evidence snapshots. |
| crates/cmtraceopen-parser/src/sccm/client/mod.rs | Introduces SccmNormalizedBundle as normalized input for client analyzers. |
| crates/cmtraceopen-parser/src/sccm/client/deployment.rs | Implements the deployment transaction reducer, findings, and counterpart-ready handoff fact. |
| crates/cmtraceopen-parser/src/sccm/catalog.rs | Adds StateMessage client catalog entry (logical name stateMessage). |
Comment on lines
+173
to
185
| #[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] | ||
| #[serde(rename_all = "camelCase")] | ||
| pub enum SccmRecordCompleteness { | ||
| LogicalRecord, | ||
| PhysicalFragment, | ||
| } | ||
|
|
||
| #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] | ||
| #[serde(rename_all = "camelCase")] | ||
| pub struct SccmEvidence { | ||
| pub evidence_id: String, | ||
| pub completeness: SccmRecordCompleteness, | ||
| pub reference: SccmEvidenceRef, |
Owner
Author
There was a problem hiding this comment.
Declining this one, and leaving the thread open so it can be argued.
The premise does not hold in this repository. SccmEvidence is producer-only: it is constructed in process by normalize_ccm_artifact and normalize_physical_lines, or by hand in tests. Nothing deserializes it. I checked every path the comment names:
- persisted fixtures: the SCCM fixtures are
manifest.jsonplus raw.logbytes, andload_bundlerebuilds evidence from those bytes on every run. No fixture stores serializedSccmEvidence. - caches: there is no cache of normalized bundles anywhere in the crate.
- IPC: the SCCM module is not wired into the Tauri surface at all. The only
sccmhit undersrc-tauri/srcis the word "SCCM" in a doc string incommands/known_sources.rs.
So there is no previously serialized evidence to be compatible with, and no forward path that would produce any.
More importantly, the specific default suggested would be actively unsafe here. completeness is a fail-closed gate, not a descriptive tag: admitted_source in client/deployment.rs refuses to promote a PhysicalFragment into a fact, and its doc comment says completeness is read from the record and never inferred from the artifact. #[serde(default)] returning LogicalRecord makes an absent field mean "this is a complete record", so any input that omits the field, whether truncated, hand-written, or produced by an older writer, would silently promote fragments into facts. That is exactly the fragment-promotion hole closed at d10ad8f7, reopened through the deserializer.
If SccmEvidence ever does gain a persistence path, the right answer is an explicit schema version on the envelope and a hard error on a missing completeness, which fails closed. I would rather add that when there is something to be compatible with than add a permissive default now that quietly weakens the gate.
Happy to reverse this if you can point at a deserialization site I missed.
Facts are in canonical reference order, which is artifact-major and not chronological, so first_fact could pair a start from one attempt with a completion from another. conclude then found the chain unorderable and downgraded a confirmed enforcement failure to insufficient evidence. Elect the transfer pair together and every other chain record against the current chain tail, falling back to the first of the kind so a genuinely unorderable set still fails closed. Walk the covered line spans once in normalize_physical_lines instead of rescanning every record for every line. Refs #322
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
Suppressed comments (2)
crates/cmtraceopen-parser/src/sccm/client/deployment.rs:1962
observation_reasonreturns a message that specifically mentions "supplemental text" whencomplete_logical_recordis true, but this branch also applies to complete CCM logical records that were not admitted (e.g., due to version/profile gating). Using a more general message avoids misleading consumers about the evidence type.
return "unvalidated supplemental text cannot override an exact keyed client transaction";
crates/cmtraceopen-parser/src/sccm/client/deployment.rs:614
- The doc comment for
combine_coveragesays conflicting non-capture states becomeParseFailed, but the implementation deliberately prefersCapped(and thenPartial) when any artifact has those states. This mismatch makes it hard for readers to understand the intended precedence rules without re-deriving them from the code.
/// Any complete capture makes the group usable; otherwise the most explanatory
/// incomplete state wins. Conflicting noncapture states stay `ParseFailed` so
/// no caller can read a single cause out of a mixed group.
This was referenced Aug 1, 2026
adamgell
added a commit
that referenced
this pull request
Aug 1, 2026
) The CI workflow triggers only on `main`, but the entire SCCM program stacks on `codex/parser-family-skeleton`. Seven open pull requests (#391, #392, #394, #404, #405, #407, #420) therefore run none of the six jobs -- no cargo check/test, no clippy, no MSRV check, no tsc, no Playwright E2E, and no platform builds -- while still reporting as mergeable. GitHub gives no signal for this: a pull request whose base branch is absent from the trigger list is not "pending" or "skipped", it simply has no checks, which is easy to misread as a passing PR with light gating. Add the integration branch to both triggers so lane PRs are gated before they land on it, and so the branch itself is checked after each merge. Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This was referenced Aug 1, 2026
Owner
Author
|
Superseded on main. Current deployment analysis uses sealed admission, retains all 12 corpus scenarios, and expands the direct suite from 32 to 37 with recovery, equal-time, ambiguity, and authority regressions; latest correction 61a4256. Promoted through PR #490. Closing this retired draft unmerged. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements the client application, package, and content deployment transaction analyzer for #322, specified by the already-merged deployment corpus.
Public API
New
sccm/client/deployment.rs, re-exported throughcmtraceopen_parser::sccm:analyze_client_deployment(&SccmNormalizedBundle) -> SccmDeploymentAnalysisAn eight-phase workflow (Intent, Requirements, LocateContent, Transfer, Cache, Enforce, Detect, Report) with transactions, keys, counterpart facts, timestamp provenance, artifact requests, coverage, source-local observations, findings, an extraction profile, and a correlation handoff. All serialized names are camelCase and additive.
Corpus coverage: 12 of 12 scenarios reproduce exactly
Each scenario matches the declared transactions (id, key including profile kind, confidence and every optional field, phase, state, lastSuccessfulPhase, classification, confidence, ceiling, coverage-gap artifact ids, next-artifact group and verbatim reason, evidence spans), coverage rows, source-local observations, declared findings, and counterpart-ready facts. Counterpart-ready facts appear in exactly the six declared scenarios and nowhere else, and the correlation handoff reports all four flags false in all twelve, so no cross-side claim is made anywhere.
Worth noting: the
successscenario carries policy coverageabsent, which proves the reducer stays conservative without policy facts, satisfying the plan's requirement that #322 must not depend on #321 output.Shared spine additions
Three changes land outside the lane file and each is intentional:
SccmCoverageState::Partialinmodels.rs, with its ordering arms infindings.rsandserver/windows/intake.rs.normalize_physical_linesiningest.rs, emitting only lines that no complete logical record covers, with no timestamp and no component, so a fragment can be cited but never ordered or promoted.StateMessagecatalog entry. Without itclassify_artifact_namereturnssupported_for_diagnosis: falseand thesuccessscenario never reaches the report phase. This was caught mid-implementation by the corpus itself:successreturnedinsufficientEvidenceuntil the entry existed.The first two are identical in intent to changes already sitting on the #320 health branch, so whichever of the two PRs merges second needs a small conflict resolution. Flagging that explicitly rather than letting a reviewer discover it.
Verification
--test sccm_client_deployment: 22 passed--test sccm_client_deployment_fixture_contract: 8 passed--test sccm_spine_contract: 137 passedcargo test --locked -p cmtraceopen-parser: 947 passed, 0 failedcargo clippy --locked -p cmtraceopen-parser --all-targets -- -D warnings: cleancargo check --locked -p cmtraceopen-parser --target wasm32-unknown-unknown: cleangit diff --check 26116027 HEADclean;rustfmt --checkclean on all 11 changed files; no dependency changesRED/GREEN
Five pairs. Three had genuine compile or assertion failures at their RED commit. Pair 3 passed on arrival because the guards were built in pairs 1 and 2, so it was instead proven by eight targeted mutations, each failing exactly its own test and then reverted: dropping the client-role filter on the artifact map, disabling the identity-collision guard, assuming every chain ordered, letting a duplicated exact-token label pick the first value, letting an ambiguous key pick the lowest, dropping the version-profile gate, accepting a nonzero exit code without a terminal flag, and matching an event phrase anywhere in the clause. Pair 4 is a characterization of the public camelCase projection.
Deliberately not implemented
SccmNormalizedBundlecarries onlySccmEvidence, and admitting parser-specific outputs is a bundle-shape decision belonging to SCCM Client: add deterministic intake, coverage, and corpus foundation #319 intake; inventing it here would fork the struct away from SCCM Client: diagnose setup, health, identity, assignment, and location #320.extract_signalsanderror_dbdescriptions are not projected onto deployment output. No corpus scenario asserts it.Where the reducer deliberately differs from the corpus
Reported rather than silently matched, because these are cases where the corpus declares capability rather than observation:
extractionProfile.keyKindsmatches in 8 of 12. Four scenarios declare the profile's full ten-kind capability list rather than what they observed; the reducer reports observed kinds.extractionProfile.validatedArtifactFamiliesmatches in 7 of 12.rotation-boundaryadditionally declaresclient-contentvalidated although neither rotation fragment ever formed a record; the reducer reports only families that yielded an admitted record.fragment:<artifactId>,supplemental:<artifactId>) and match the corpus exactly for both fragment cases. The corpus'ssupplemental:installer:enforcement-exitembeds a scenario name that is not derivable from a bundle.findingsare non-exhaustive, declaring none for the bits, cache, and enforcement failures. The reducer emits one finding per distinct cause; the test asserts every declared finding is reproduced field for field including its prohibitions, and separately validates every produced finding against the shared contract.nextArtifactuses corpus group ids, while a finding'snext_artifactsmust useSccmArtifactRequest, whose shared validator accepts only declared catalog logical ids with a reason naming that artifact. The corpus's group-shaped requests would be rejected by the merged spine.Refs #322. No native Windows acceptance is claimed; this is pure parser-layer work.
🤖 Generated with Claude Code
Summary by CodeRabbit
StateMessage.logclient evidence.