feat(sccm): add bounded advanced server capture - #500
Conversation
github-actions
Bot
added
enhancement
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds bounded advanced SCCM server-log capture across the workspace, Tauri commands, native collector, manifest generation, and parser intake. The flow uses allowlisted sources, expiring single-use capabilities, provenance-aware contracts, filesystem safety checks, and parser-ineligible advanced artifacts. ChangesAdvanced SCCM capture
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant SccmWorkspace
participant TauriCommands
participant SccmAdvancedCapabilityStore
participant AdvancedCollector
participant Parser
SccmWorkspace->>TauriCommands: submit selected source and root
TauriCommands->>SccmAdvancedCapabilityStore: validate and authorize request
SccmWorkspace->>TauriCommands: submit opaque capability handle
TauriCommands->>SccmAdvancedCapabilityStore: consume handle
TauriCommands->>AdvancedCollector: capture allowlisted files
AdvancedCollector-->>TauriCommands: return bounded bundle and manifest
TauriCommands-->>SccmWorkspace: return capture result
Parser->>Parser: validate capture contract and integrity bindings
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
Hosted CI rework is active. The failure is confined to cargo test --no-default-features: the new sccm_advanced_ipc and sccm_advanced_server_capture integration targets import the feature-gated SCCM module but lack the existing Cargo required-features metadata pattern. The fix adds only those two test declarations, then reruns no-default and focused feature tests. Product/full-feature checks remain green. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/cmtraceopen-parser/src/sccm/server/windows/intake.rs (1)
729-774: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winDocument the new public items and note the enum variant addition.
SccmServerCaptureContract,SccmServerAdvancedProvenance,SccmServerConfiguredPathClass, and the newSccmServerConfiguredPathState::OperatorDeclaredvariant are public API of a published crate and carry no rustdoc. Add///docs that state the meaning of each field and variant, in particular thatcapability_handleandauthorization_handleare opaque digests and never raw values.Adding
OperatorDeclaredtoSccmServerConfiguredPathStateand adding variants to a public enum breaks downstream exhaustivematcharms. If external consumers match on these enums, add#[non_exhaustive]now or record the change in the crate changelog.As per path instructions: "Treat every public item as a semver commitment: flag breaking changes to public types, signatures, or enum variants, and check that new public items are documented."
🤖 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/server/windows/intake.rs` around lines 729 - 774, Document the public types SccmServerCaptureContract, SccmServerAdvancedProvenance, and SccmServerConfiguredPathClass with rustdoc covering every field and variant, explicitly describing capability_handle and authorization_handle as opaque digests rather than raw values. Document the OperatorDeclared variant added to SccmServerConfiguredPathState, and mark affected public enums #[non_exhaustive] or record the exhaustive-match breaking change in the crate changelog.Source: Path instructions
🤖 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/server/windows/intake.rs`:
- Around line 2198-2219: Extract the eight-field capture-contract byte
accounting from coverage_string_bytes into a shared
checked_add_capture_contract_bytes helper accepting &mut usize and
&SccmServerCaptureContract and returning Option<()>. Replace the duplicated
inline accounting at both call sites with this helper, preserving existing
checked-add error propagation and provenance string mappings.
- Around line 2858-2863: Update the advancedCapture match arm near the
collection-limit check to compare byte_limit with
ADVANCED_CAPTURE_SOURCE_BYTE_LIMIT and replace the repeated Some(2) value with a
shared named file-limit constant. Reuse those constants in
normalize_advanced_capture_contract and the budget check so all advanced-capture
gates remain consistent.
- Around line 3117-3131: Replace the Debug-based provenance formatting in
capture_contract_identity with a dedicated explicit provenance key function,
following the existing role_sort_key and coverage_sort_key patterns. Reuse that
function for both role_provenance and path_provenance in
capture_contract_identity and in the two checked_add_string_bytes blocks, so
duplicate detection, coverage keys, and integrity digests use stable canonical
values.
In `@crates/cmtraceopen-parser/tests/sccm_server_intake.rs`:
- Around line 133-142: Update the tampered fixture in
advanced_capture_rejects_budget_evasion_by_tampered_handles so both artifacts
use 2 MiB when tamper_handles is true, keeping the total within the byte budget.
Ensure the untampered 2 MiB pair is asserted to succeed, so rejection is
attributable only to the mismatched capabilityHandle and authorizationHandle
values rather than the budget check.
- Around line 19-104: Extend the advanced-capture test fixtures around
advanced_capture_manifest with an observed-provenance case using a recognized
advanced producer role, matching rolesObserved, configured path state, and
pathClass equal to pathClaim. Add behavioral negative tests covering a
mismatched path class and rolesObserved containing an advanced role without a
corresponding advancedCapture artifact, asserting intake rejects each case.
Ensure assertions validate accepted or rejected intake outcomes rather than
merely checking fixture fields.
In `@docs/superpowers/plans/2026-08-04-sccm-advanced-server-capture.md`:
- Line 256: Update the post-SUP action statement to reference only Task 1’s
defined red UI/IPC tests, removing “parser-contract tests” while keeping parser
intake verification assigned to Task 4.
- Line 5: Replace all Unicode em dash and en dash characters in the Markdown
plan with ASCII hyphens or colons, including the issue range and normative UI
labels. Update the corresponding frontend label strings in SccmWorkspace.tsx so
the plan and UI remain identical, and ensure the affected locations at the
referenced lines contain no Unicode dash characters.
- Line 141: Revise Step 2’s rejection tests so they no longer require the
capability store to be empty for every rejection. Keep an explicit empty-store
rejection case, and add a seeded-valid-entry scenario that rejects a second
authorization while asserting the original handle and store count remain
unchanged.
- Around line 109-113: Bind operator-declared roots in
authorize_sccm_advanced_capture to backend-trusted picker consent or a strict
validated-root allowlist before capability creation. Reject any selected_root
without valid backend-issued authorization, and add a negative test proving a
guessed readable directory cannot obtain a capability or capture files.
In `@src-tauri/src/sccm/collector/advanced_capture.rs`:
- Around line 805-809: Update the rotation object’s kind field to call the
existing rotation_segment helper instead of duplicating the inline match on
rotation. Preserve the current mapping and pass the same rotation value/context
required by rotation_segment.
- Around line 970-981: Update the capped calculation in the advanced capture
read flow so a short read caused by concurrent file rotation is treated as an
uncapped bounded fragment rather than byte-limit truncation. Base the final
capped status on the bytes actually read, while retaining take(read_limit) to
bound growth and removing the short-read failure path in this block.
In `@src-tauri/tests/sccm_advanced_ipc.rs`:
- Around line 132-167: Add a focused test alongside
capability_store_is_bounded_and_purges_before_authorize that authorizes the same
root and role twice at the same timestamp, asserting the second
SccmAdvancedCapabilityStore::authorize call fails and pending_count remains 1.
Use a single temporary root so the duplicate-identity rejection path is
exercised.
- Around line 72-76: Update the glob assertion in the advanced IPC test to use
an existing directory whose name contains a glob metacharacter, ensuring
validate_selected_root reaches the metacharacter gate rather than failing
symlink_metadata. Keep the test platform-aware because such names are
unavailable on Windows, gating the case or using an equivalent Unix-only setup.
In `@src-tauri/tests/sccm_advanced_server_capture.rs`:
- Around line 114-119: Replace the ineffective manifest-content and source-path
assertions in the advanced capture test with checks against actual files in the
on-disk bundle. Add a recursive `walk_files` helper and assert that no collected
evidence file has the decoy basenames `CloudMgr.log.1` or `CloudMgr-copy.log`,
while preserving the existing `artifact_count` and `retained_bytes` assertions.
In `@src/lib/commands.test.ts`:
- Around line 159-162: Remove the redundant negative assertion for
"capture_sccm_advanced_diagnostics" and rely on the existing exact
toHaveBeenNthCalledWith assertion that verifies the second invoke call contains
only capabilityHandle.
In `@src/workspaces/sccm/SccmWorkspace.test.tsx`:
- Around line 334-381: Add a test in the SccmWorkspace advanced-capture tests
covering captureSccmAdvancedDiagnostics rejection after
authorizeSccmAdvancedCapture resolves. Assert cancelSccmAdvancedCapture receives
the issued capabilityHandle, the capture error appears in the alert, and
useSccmStore advancedCapability is cleared.
In `@src/workspaces/sccm/SccmWorkspace.tsx`:
- Around line 568-573: Update the button aria-label in the advanced capture UI
near captureAdvanced to include option.sourceId alongside the existing card
label, ensuring each source has a unique accessible name. Preserve the current
fallback behavior for ADVANCED_CARD_LABELS and update all affected button
queries in SccmWorkspace.test.tsx to match the new accessible names.
- Around line 336-340: Update the declined-consent branch in the advanced SCCM
capture flow to await cancelSccmAdvancedCapture with local error handling that
prevents cancellation failures from reaching the capture-failed catch path or
displaying a capture error. Request cancellation before calling
clearAdvancedCapability, while preserving the early return and existing behavior
for accepted consent.
In `@src/workspaces/sccm/types.ts`:
- Around line 84-95: The SccmAdvancedSourceOption contract must expose a
backend-resolved role/path tuple rather than independent roleScopes and
pathClasses arrays. Update the interface and its producers/consumers so each
option provides the exact role and path selected for the observed fact, and
update SccmWorkspace.tsx to use that tuple instead of indexing the separate
arrays, preserving the managementPoint behavior through the resolved value.
---
Outside diff comments:
In `@crates/cmtraceopen-parser/src/sccm/server/windows/intake.rs`:
- Around line 729-774: Document the public types SccmServerCaptureContract,
SccmServerAdvancedProvenance, and SccmServerConfiguredPathClass with rustdoc
covering every field and variant, explicitly describing capability_handle and
authorization_handle as opaque digests rather than raw values. Document the
OperatorDeclared variant added to SccmServerConfiguredPathState, and mark
affected public enums #[non_exhaustive] or record the exhaustive-match breaking
change in the crate changelog.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1441c4ac-4276-40e5-8e2f-9902c8912f3c
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!Cargo.lock
📒 Files selected for processing (20)
crates/cmtraceopen-parser/src/sccm/server/windows/intake.rscrates/cmtraceopen-parser/tests/sccm_server_intake.rsdocs/superpowers/plans/2026-08-04-sccm-advanced-server-capture.mdsrc-tauri/Cargo.tomlsrc-tauri/src/commands/sccm.rssrc-tauri/src/lib.rssrc-tauri/src/sccm/collector/advanced_capture.rssrc-tauri/src/sccm/collector/discovery.rssrc-tauri/src/sccm/collector/mod.rssrc-tauri/src/state/app_state.rssrc-tauri/tests/sccm_advanced_ipc.rssrc-tauri/tests/sccm_advanced_server_capture.rssrc/lib/commands.test.tssrc/lib/commands.tssrc/workspaces/sccm/SccmWorkspace.test.tsxsrc/workspaces/sccm/SccmWorkspace.tsxsrc/workspaces/sccm/sccm-store.test.tssrc/workspaces/sccm/sccm-store.tssrc/workspaces/sccm/sccm-workspace.csssrc/workspaces/sccm/types.ts
| if let Some(contract) = &artifact.capture_contract { | ||
| checked_add_string_bytes(&mut total, &contract.card_id)?; | ||
| checked_add_string_bytes(&mut total, &contract.card_version)?; | ||
| checked_add_string_bytes(&mut total, &contract.capability_handle)?; | ||
| checked_add_string_bytes(&mut total, &contract.authorization_handle)?; | ||
| checked_add_string_bytes(&mut total, &contract.role_claim)?; | ||
| checked_add_string_bytes(&mut total, &contract.path_claim)?; | ||
| checked_add_string_bytes( | ||
| &mut total, | ||
| match contract.role_provenance { | ||
| SccmServerAdvancedProvenance::Observed => "observed", | ||
| SccmServerAdvancedProvenance::OperatorDeclared => "operatorDeclared", | ||
| }, | ||
| )?; | ||
| checked_add_string_bytes( | ||
| &mut total, | ||
| match contract.path_provenance { | ||
| SccmServerAdvancedProvenance::Observed => "observed", | ||
| SccmServerAdvancedProvenance::OperatorDeclared => "operatorDeclared", | ||
| }, | ||
| )?; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Extract the capture-contract byte accounting into one helper.
The eight-field accounting here is duplicated verbatim in coverage_string_bytes. A field added to SccmServerCaptureContract later will be counted in one place and missed in the other, which silently changes the integrity digest input without a compile error. Add fn checked_add_capture_contract_bytes(total: &mut usize, contract: &SccmServerCaptureContract) -> Option<()> and call it from both sites.
🤖 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/server/windows/intake.rs` around lines
2198 - 2219, Extract the eight-field capture-contract byte accounting from
coverage_string_bytes into a shared checked_add_capture_contract_bytes helper
accepting &mut usize and &SccmServerCaptureContract and returning Option<()>.
Replace the duplicated inline accounting at both call sites with this helper,
preserving existing checked-add error propagation and provenance string
mappings.
| _ if artifact.source_kind == "advancedCapture" | ||
| && artifact.collection_limit.as_ref().is_some_and(|limit| { | ||
| limit.byte_limit == 4 * 1024 * 1024 | ||
| && limit.file_limit == Some(2) | ||
| && !limit.limit_applied | ||
| }) => {} |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use ADVANCED_CAPTURE_SOURCE_BYTE_LIMIT instead of the literal.
normalize_advanced_capture_contract compares byte_limit against ADVANCED_CAPTURE_SOURCE_BYTE_LIMIT. This arm hardcodes 4 * 1024 * 1024. If the constant changes, the two gates disagree and non-physical advanced artifacts fail with UnexpectedPayload for a limit the contract normalizer accepts. Also hoist the file limit to a named constant, since Some(2) is repeated in normalize_advanced_capture_contract and in the budget check.
♻️ Proposed change
_ if artifact.source_kind == "advancedCapture"
&& artifact.collection_limit.as_ref().is_some_and(|limit| {
- limit.byte_limit == 4 * 1024 * 1024
- && limit.file_limit == Some(2)
+ limit.byte_limit == ADVANCED_CAPTURE_SOURCE_BYTE_LIMIT
+ && limit.file_limit == Some(ADVANCED_CAPTURE_SOURCE_FILE_LIMIT)
&& !limit.limit_applied
}) => {}🤖 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/server/windows/intake.rs` around lines
2858 - 2863, Update the advancedCapture match arm near the collection-limit
check to compare byte_limit with ADVANCED_CAPTURE_SOURCE_BYTE_LIMIT and replace
the repeated Some(2) value with a shared named file-limit constant. Reuse those
constants in normalize_advanced_capture_contract and the budget check so all
advanced-capture gates remain consistent.
| fn capture_contract_identity(contract: Option<&SccmServerCaptureContract>) -> String { | ||
| contract.map_or_else(String::new, |contract| { | ||
| format!( | ||
| "{}\0{}\0{}\0{}\0{}\0{}\0{:?}\0{:?}", | ||
| contract.card_id, | ||
| contract.card_version, | ||
| contract.capability_handle, | ||
| contract.authorization_handle, | ||
| contract.role_claim, | ||
| contract.path_claim, | ||
| contract.role_provenance, | ||
| contract.path_provenance, | ||
| ) | ||
| }) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not build a canonical identity from Debug output.
capture_contract_identity formats role_provenance and path_provenance with {:?}. This string feeds duplicate detection, the coverage key, and the integrity digest. Debug is not a stability contract. A variant rename or a hand-written Debug impl changes canonical identities and the digest with no compile error and no test failure. Every other identity in this file uses an explicit key function such as role_sort_key or coverage_sort_key. Add one for provenance and use it here and in the two checked_add_string_bytes blocks.
🐛 Proposed fix
+fn advanced_provenance_key(provenance: SccmServerAdvancedProvenance) -> &'static str {
+ match provenance {
+ SccmServerAdvancedProvenance::Observed => "observed",
+ SccmServerAdvancedProvenance::OperatorDeclared => "operatorDeclared",
+ }
+}
+
fn capture_contract_identity(contract: Option<&SccmServerCaptureContract>) -> String {
contract.map_or_else(String::new, |contract| {
format!(
- "{}\0{}\0{}\0{}\0{}\0{}\0{:?}\0{:?}",
+ "{}\0{}\0{}\0{}\0{}\0{}\0{}\0{}",
contract.card_id,
contract.card_version,
contract.capability_handle,
contract.authorization_handle,
contract.role_claim,
contract.path_claim,
- contract.role_provenance,
- contract.path_provenance,
+ advanced_provenance_key(contract.role_provenance),
+ advanced_provenance_key(contract.path_provenance),
)
})
}🤖 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/server/windows/intake.rs` around lines
3117 - 3131, Replace the Debug-based provenance formatting in
capture_contract_identity with a dedicated explicit provenance key function,
following the existing role_sort_key and coverage_sort_key patterns. Reuse that
function for both role_provenance and path_provenance in
capture_contract_identity and in the two checked_add_string_bytes blocks, so
duplicate detection, coverage keys, and integrity digests use stable canonical
values.
| fn advanced_capture_manifest( | ||
| state: &str, | ||
| include_payload: bool, | ||
| ) -> (String, Vec<SccmServerArtifactPayload>) { | ||
| let digest = "a".repeat(64); | ||
| let artifact_id = format!("cmtraceopen.artifact.sha256.v1:{digest}"); | ||
| let capability = format!( | ||
| "cmtraceopen.capture-capability.sha256.v1:{}", | ||
| "b".repeat(64) | ||
| ); | ||
| let authorization = format!( | ||
| "cmtraceopen.capture-authorization.sha256.v1:{}", | ||
| "c".repeat(64) | ||
| ); | ||
| let root_handle = format!("root-{}", "d".repeat(64)); | ||
| let relative_path = include_payload.then(|| { | ||
| format!( | ||
| "evidence/sccm/server/unclassified/advanced-osd-pxe/{root_handle}/current/smspxe.log" | ||
| ) | ||
| }); | ||
| let manifest = serde_json::json!({ | ||
| "sccmManifestVersion": 1, | ||
| "syntheticFixture": false, | ||
| "proposalOnly": null, | ||
| "privacy": { "synthetic": false, "rawPaths": "redacted" }, | ||
| "bundleRole": "server", | ||
| "topology": { | ||
| "captureHost": format!("cmtraceopen.host.sha256.v1:{}", "e".repeat(64)), | ||
| "siteCode": format!("cmtraceopen.site.sha256.v1:{}", "f".repeat(64)), | ||
| "rolesObserved": ["siteServer"], | ||
| "hierarchyLinks": [] | ||
| }, | ||
| "inputOrderIsDeliberatelyUnsorted": null, | ||
| "artifacts": [{ | ||
| "artifactId": artifact_id, | ||
| "producerRole": "unclassified", | ||
| "producerHostHandle": format!("cmtraceopen.host.sha256.v1:{}", "e".repeat(64)), | ||
| "workflowSubject": null, | ||
| "sourceId": "advanced-osd-pxe", | ||
| "sourceKind": "advancedCapture", | ||
| "sourceVersion": "5.00.9141.1000", | ||
| "originalPath": "REDACTED", | ||
| "originalBasename": "smspxe.log", | ||
| "configuredPathProvenance": { | ||
| "state": "operatorDeclared", | ||
| "pathClass": null, | ||
| "pathFingerprint": format!("cmtraceopen.path.sha256.v1:{}", "1".repeat(64)) | ||
| }, | ||
| "captureContract": { | ||
| "cardId": "osd-pxe", | ||
| "cardVersion": "1.0.0", | ||
| "capabilityHandle": capability, | ||
| "authorizationHandle": authorization, | ||
| "roleClaim": "distributionPointPxe", | ||
| "pathClaim": "configuredRoleLogRoot", | ||
| "roleProvenance": "operatorDeclared", | ||
| "pathProvenance": "operatorDeclared" | ||
| }, | ||
| "defaultCandidateState": null, | ||
| "rotation": { | ||
| "kind": "current", | ||
| "value": null, | ||
| "lineageId": format!("cmtraceopen.lineage.sha256.v1:{}", "2".repeat(64)) | ||
| }, | ||
| "captureState": state, | ||
| "collectionDetail": null, | ||
| "skipReason": null, | ||
| "unsupportedReason": (state == "unsupported").then(|| format!("cmtraceopen.unsupported-reason.sha256.v1:{}", "3".repeat(64))), | ||
| "encoding": include_payload.then_some("unknown"), | ||
| "collectionLimit": { "byteLimit": 4194304, "fileLimit": 2, "limitApplied": false }, | ||
| "truncated": null, | ||
| "fragmentComplete": null, | ||
| "collectedUtc": "2026-08-04T20:00:00Z", | ||
| "relativePath": relative_path, | ||
| "bytesCopied": if include_payload { 8 } else { 0 } | ||
| }] | ||
| }); | ||
| let payloads = include_payload | ||
| .then(|| SccmServerArtifactPayload { | ||
| manifest_artifact_id: artifact_id, | ||
| bytes: b"captured".to_vec(), | ||
| }) | ||
| .into_iter() | ||
| .collect(); | ||
| (serde_json::to_string(&manifest).unwrap(), payloads) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add an observed-provenance fixture.
Every advanced fixture in this file uses roleProvenance and pathProvenance of operatorDeclared. The observed branch of normalize_advanced_capture_contract is never executed. That branch carries the strictest checks in the change: role_sort_key(producer_role) == role_claim, path state limited to configured or supplied, and path_class == path_claim. The advanced-role topology gate at intake.rs lines 855 to 867 and the new SccmServerConfiguredPathClass variants are also unexercised.
Add a fixture with producerRole set to a recognized advanced role, that role present in rolesObserved, configuredPathProvenance.state of configured, and pathClass equal to the pathClaim. Then add negative cases for a mismatched pathClass and for an advanced role in rolesObserved with no advancedCapture artifact.
As per path instructions: "Verify assertions test real behavior rather than restating the implementation."
🤖 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_server_intake.rs` around lines 19 - 104,
Extend the advanced-capture test fixtures around advanced_capture_manifest with
an observed-provenance case using a recognized advanced producer role, matching
rolesObserved, configured path state, and pathClass equal to pathClaim. Add
behavioral negative tests covering a mismatched path class and rolesObserved
containing an advanced role without a corresponding advancedCapture artifact,
asserting intake rejects each case. Ensure assertions validate accepted or
rejected intake outcomes rather than merely checking fixture fields.
Source: Path instructions
| if tamper_handles { | ||
| overflow["captureContract"]["capabilityHandle"] = json!(format!( | ||
| "cmtraceopen.capture-capability.sha256.v1:{}", | ||
| "e".repeat(64) | ||
| )); | ||
| overflow["captureContract"]["authorizationHandle"] = json!(format!( | ||
| "cmtraceopen.capture-authorization.sha256.v1:{}", | ||
| "f".repeat(64) | ||
| )); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
The tamper test does not test the handle check.
With tamper_handles == true the overflow artifact still carries 4 MiB and the base artifact carries 8 bytes. The group total is 4194312 bytes, so the byte cap at intake.rs line 1096 rejects the bundle on its own. Delete the handle equality check at intake.rs lines 1085 to 1089 and advanced_capture_rejects_budget_evasion_by_tampered_handles still passes. The test restates a rejection it cannot attribute.
Make the tampered fixture fit inside the budget so only the handle mismatch can reject it. Set both artifacts to 2 MiB when tamper_handles is true, and assert the untampered 2 MiB pair succeeds.
As per path instructions: "Verify assertions test real behavior rather than restating the implementation."
🤖 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_server_intake.rs` around lines 133 -
142, Update the tampered fixture in
advanced_capture_rejects_budget_evasion_by_tampered_handles so both artifacts
use 2 MiB when tamper_handles is true, keeping the total within the byte budget.
Ensure the untampered 2 MiB pair is asserted to succeed, so rejection is
attributable only to the mismatched capabilityHandle and authorizationHandle
values rather than the budget check.
Source: Path instructions
| expect(invoke).not.toHaveBeenCalledWith( | ||
| "capture_sccm_advanced_diagnostics", | ||
| expect.objectContaining({ selectedRoot: expect.anything() }), | ||
| ); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
This negative assertion is redundant and keyed on one field name.
Line 151-155 already pins the second invoke call to exactly { capabilityHandle: capability.capabilityHandle }. toHaveBeenNthCalledWith is an exact match, so any extra key already fails it. The objectContaining({ selectedRoot }) guard adds no coverage.
It also implies a stronger property than it enforces. If captureSccmAdvancedDiagnostics started forwarding the path under any other key, such as rootPath or root, this assertion would still pass. Delete it and rely on the exact-match assertion above.
♻️ Proposed change
expect(invoke).toHaveBeenNthCalledWith(3, "cancel_sccm_advanced_capture", {
capabilityHandle: capability.capabilityHandle,
});
- expect(invoke).not.toHaveBeenCalledWith(
- "capture_sccm_advanced_diagnostics",
- expect.objectContaining({ selectedRoot: expect.anything() }),
- );
});📝 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.
| expect(invoke).not.toHaveBeenCalledWith( | |
| "capture_sccm_advanced_diagnostics", | |
| expect.objectContaining({ selectedRoot: expect.anything() }), | |
| ); | |
| expect(invoke).toHaveBeenNthCalledWith(3, "cancel_sccm_advanced_capture", { | |
| capabilityHandle: capability.capabilityHandle, | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/commands.test.ts` around lines 159 - 162, Remove the redundant
negative assertion for "capture_sccm_advanced_diagnostics" and rely on the
existing exact toHaveBeenNthCalledWith assertion that verifies the second invoke
call contains only capabilityHandle.
| it("cancels a capability when final consent is declined", async () => { | ||
| const advancedDiscovery: SccmEnvironmentDiscovery = { | ||
| ...DISCOVERY, | ||
| advancedSources: [ | ||
| { | ||
| cardId: "client-notification-bgb", | ||
| cardVersion: "1.0.0", | ||
| sourceId: "advanced-client-notification-bgb", | ||
| roleScopes: ["clientNotificationServer", "managementPoint"], | ||
| pathClasses: ["configuredRoleLogRoot", "siteServerLogs"], | ||
| sourceVersion: null, | ||
| availability: "observed", | ||
| maxBytes: 4_194_304, | ||
| maxFiles: 2, | ||
| rotations: ["current", "lo_"], | ||
| }, | ||
| ], | ||
| }; | ||
| const capability = { | ||
| capabilityHandle: `cmtraceopen.capture-capability.sha256.v1:${"b".repeat(64)}`, | ||
| cardId: "client-notification-bgb", | ||
| cardVersion: "1.0.0", | ||
| sourceId: "advanced-client-notification-bgb", | ||
| roleScope: "managementPoint", | ||
| pathClass: "configuredRoleLogRoot", | ||
| sourceVersion: null, | ||
| }; | ||
| vi.mocked(discoverSccmEnvironment).mockResolvedValue(advancedDiscovery); | ||
| vi.mocked(open).mockResolvedValue("C:\\private-root"); | ||
| vi.spyOn(window, "confirm").mockReturnValueOnce(true).mockReturnValueOnce(false); | ||
| vi.mocked(authorizeSccmAdvancedCapture).mockResolvedValue(capability); | ||
| render(<SccmWorkspace />); | ||
|
|
||
| fireEvent.click(screen.getByRole("button", { name: "Discover SCCM environment" })); | ||
| fireEvent.click( | ||
| await screen.findByRole("button", { | ||
| name: "Authorize Client notification / BGB bounded capture", | ||
| }), | ||
| ); | ||
|
|
||
| await waitFor(() => | ||
| expect(cancelSccmAdvancedCapture).toHaveBeenCalledWith( | ||
| capability.capabilityHandle, | ||
| ), | ||
| ); | ||
| expect(captureSccmAdvancedDiagnostics).not.toHaveBeenCalled(); | ||
| expect(useSccmStore.getState().advancedCapability).toBeNull(); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add coverage for the capture-failure cancellation branch.
The four new tests cover declined consent, unmount, and operation replacement. They do not cover the catch branch at lines 346-355 of src/workspaces/sccm/SccmWorkspace.tsx. That branch is the only path that cancels a capability after captureSccmAdvancedDiagnostics rejects, and it depends on issuedCapabilityHandle being assigned before the failure.
A rejected capture leaves a live single-use capability in the native store until its five-minute TTL expires. That is the exact condition this PR is built to prevent, so the branch deserves a test.
💚 Proposed test
it("cancels the capability when advanced capture fails", async () => {
const advancedDiscovery: SccmEnvironmentDiscovery = {
...DISCOVERY,
advancedSources: [
{
cardId: "reporting",
cardVersion: "1.0.0",
sourceId: "advanced-reporting",
roleScopes: ["reportingServicesPoint"],
pathClasses: ["configuredRoleLogRoot"],
sourceVersion: null,
availability: "operatorDeclaredCandidate",
maxBytes: 4_194_304,
maxFiles: 2,
rotations: ["current", "lo_"],
},
],
};
const capability = {
capabilityHandle: `cmtraceopen.capture-capability.sha256.v1:${"f".repeat(64)}`,
cardId: "reporting",
cardVersion: "1.0.0",
sourceId: "advanced-reporting",
roleScope: "reportingServicesPoint",
pathClass: "configuredRoleLogRoot",
sourceVersion: null,
};
vi.mocked(discoverSccmEnvironment).mockResolvedValue(advancedDiscovery);
vi.mocked(open).mockResolvedValue("C:\\private-root");
vi.spyOn(window, "confirm").mockReturnValue(true);
vi.mocked(authorizeSccmAdvancedCapture).mockResolvedValue(capability);
vi.mocked(captureSccmAdvancedDiagnostics).mockRejectedValue(
new Error("Capture destination is unavailable."),
);
render(<SccmWorkspace />);
fireEvent.click(
screen.getByRole("button", { name: "Discover SCCM environment" }),
);
fireEvent.click(
await screen.findByRole("button", {
name: "Authorize Reporting services bounded capture",
}),
);
await waitFor(() =>
expect(cancelSccmAdvancedCapture).toHaveBeenCalledWith(
capability.capabilityHandle,
),
);
expect(await screen.findByRole("alert")).toHaveTextContent(
"Capture destination is unavailable.",
);
expect(useSccmStore.getState().advancedCapability).toBeNull();
});🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/workspaces/sccm/SccmWorkspace.test.tsx` around lines 334 - 381, Add a
test in the SccmWorkspace advanced-capture tests covering
captureSccmAdvancedDiagnostics rejection after authorizeSccmAdvancedCapture
resolves. Assert cancelSccmAdvancedCapture receives the issued capabilityHandle,
the capture error appears in the alert, and useSccmStore advancedCapability is
cleared.
| if (!window.confirm("Authorization is ready. Capture this bounded source now?")) { | ||
| clearAdvancedCapability(); | ||
| await cancelSccmAdvancedCapture(capability.capabilityHandle); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
A failing cancel after a declined consent shows a capture-failed error to the operator.
This await cancelSccmAdvancedCapture(...) has no .catch(), unlike the same call on line 330 and on line 348. If it rejects, control reaches the catch on line 346. issuedCapabilityHandle is already set on line 328, so the code cancels a second time and then calls fail(errorMessage(error, "Advanced SCCM capture failed.")).
The operator declined the capture. They then see a red "Native operation failed" banner claiming the capture failed. No capture was attempted.
Also swap the order so the cancel is requested before the local state is cleared. That keeps the store and the native capability consistent if the component unmounts between the two statements.
🐛 Proposed fix
completeAdvancedAuthorization(capability);
if (!window.confirm("Authorization is ready. Capture this bounded source now?")) {
- clearAdvancedCapability();
- await cancelSccmAdvancedCapture(capability.capabilityHandle);
+ await cancelSccmAdvancedCapture(capability.capabilityHandle).catch(
+ () => undefined,
+ );
+ clearAdvancedCapability();
return;
}📝 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.
| if (!window.confirm("Authorization is ready. Capture this bounded source now?")) { | |
| clearAdvancedCapability(); | |
| await cancelSccmAdvancedCapture(capability.capabilityHandle); | |
| return; | |
| } | |
| completeAdvancedAuthorization(capability); | |
| if (!window.confirm("Authorization is ready. Capture this bounded source now?")) { | |
| await cancelSccmAdvancedCapture(capability.capabilityHandle).catch( | |
| () => undefined, | |
| ); | |
| clearAdvancedCapability(); | |
| return; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/workspaces/sccm/SccmWorkspace.tsx` around lines 336 - 340, Update the
declined-consent branch in the advanced SCCM capture flow to await
cancelSccmAdvancedCapture with local error handling that prevents cancellation
failures from reaching the capture-failed catch path or displaying a capture
error. Request cancellation before calling clearAdvancedCapability, while
preserving the early return and existing behavior for accepted consent.
| <Button | ||
| appearance={observed ? "primary" : "secondary"} | ||
| disabled={isBusy || option.availability === "blocked"} | ||
| onClick={() => void captureAdvanced(option)} | ||
| aria-label={`Authorize ${ADVANCED_CARD_LABELS[option.cardId] ?? option.cardId} bounded capture`} | ||
| > |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Two cards produce the same accessible button name.
advanced-cloud-manager and advanced-cloud-proxy-connector share cardId: "cloud-service-connection" in ADVANCED_SOURCE_CONTRACTS at lines 69-92 of src-tauri/src/sccm/collector/advanced_capture.rs. ADVANCED_CARD_LABELS maps that one id to "Cloud service connection", so both buttons render aria-label="Authorize Cloud service connection bounded capture".
A screen reader user hears two identical button names in the same grid and cannot tell which source each one captures. The visible <strong>{option.sourceId}</strong> disambiguates for sighted users only.
Include the source id in the accessible name.
🐛 Proposed fix
<Button
appearance={observed ? "primary" : "secondary"}
disabled={isBusy || option.availability === "blocked"}
onClick={() => void captureAdvanced(option)}
- aria-label={`Authorize ${ADVANCED_CARD_LABELS[option.cardId] ?? option.cardId} bounded capture`}
+ aria-label={`Authorize ${ADVANCED_CARD_LABELS[option.cardId] ?? option.cardId} (${option.sourceId}) bounded capture`}
>Update the two button queries in src/workspaces/sccm/SccmWorkspace.test.tsx at lines 308, 370, 429, and 518 to match the new name.
📝 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.
| <Button | |
| appearance={observed ? "primary" : "secondary"} | |
| disabled={isBusy || option.availability === "blocked"} | |
| onClick={() => void captureAdvanced(option)} | |
| aria-label={`Authorize ${ADVANCED_CARD_LABELS[option.cardId] ?? option.cardId} bounded capture`} | |
| > | |
| <Button | |
| appearance={observed ? "primary" : "secondary"} | |
| disabled={isBusy || option.availability === "blocked"} | |
| onClick={() => void captureAdvanced(option)} | |
| aria-label={`Authorize ${ADVANCED_CARD_LABELS[option.cardId] ?? option.cardId} (${option.sourceId}) bounded capture`} | |
| > |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/workspaces/sccm/SccmWorkspace.tsx` around lines 568 - 573, Update the
button aria-label in the advanced capture UI near captureAdvanced to include
option.sourceId alongside the existing card label, ensuring each source has a
unique accessible name. Preserve the current fallback behavior for
ADVANCED_CARD_LABELS and update all affected button queries in
SccmWorkspace.test.tsx to match the new accessible names.
| export interface SccmAdvancedSourceOption { | ||
| cardId: string; | ||
| cardVersion: string; | ||
| sourceId: string; | ||
| roleScopes: string[]; | ||
| pathClasses: string[]; | ||
| sourceVersion: string | null; | ||
| availability: SccmAdvancedSourceAvailability; | ||
| maxBytes: number; | ||
| maxFiles: number; | ||
| rotations: ["current", "lo_"]; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Expose the resolved role and path tuple.
SccmAdvancedSourceOption exposes independent roleScopes and pathClasses arrays. SccmWorkspace.tsx selects option.roleScopes[0], except for one managementPoint case, and always selects option.pathClasses[0]. The plan allows alternatives such as configuredRoleLogRoot and siteServerLogs for BgbServer.log, and multiple role scopes for cloud sources. If the observed fact uses another allowed value, observed_fact_for does not match. Authorization then rejects or downgrades the capture to operatorDeclared, which loses observed provenance.
Return one backend-selected tuple per option, or make the UI select an explicit tuple tied to the observed fact.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/workspaces/sccm/types.ts` around lines 84 - 95, The
SccmAdvancedSourceOption contract must expose a backend-resolved role/path tuple
rather than independent roleScopes and pathClasses arrays. Update the interface
and its producers/consumers so each option provides the exact role and path
selected for the observed fact, and update SccmWorkspace.tsx to use that tuple
instead of indexing the separate arrays, preserving the managementPoint behavior
through the resolved value.
|
Integration-risk gate: do not collect final Windows/MSI evidence from the current 0788929 artifact. PR #500 predates current main and overlaps PR #508/#409 in parser intake ownership. Required order: (1) accept/merge #508, establishing catalog-owned normal source IDs and privacy-safe synthetic grammars; (2) integrate #500 onto that main with serial manual judgment in |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/cmtraceopen-parser/src/sccm/server/windows/intake.rs (1)
661-680: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd Rustdoc for the capture-contract API.
Document
SccmServerCaptureContract, its public fields,SccmServerAdvancedProvenance, and its variants. Define their stable meanings for this published crate.🤖 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/server/windows/intake.rs` around lines 661 - 680, Add Rustdoc documenting the published meanings of SccmServerCaptureContract and every public field, including the distinction between role and path claims and their provenance. Also document SccmServerAdvancedProvenance and both Observed and OperatorDeclared variants with stable API semantics.Source: Path instructions
♻️ Duplicate comments (2)
crates/cmtraceopen-parser/tests/sccm_server_intake.rs (2)
133-142: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep the handle-tamper case inside the byte budget.
The over-budget fixture can reject before the capability-handle comparison runs. The test then cannot prove that tampered
capabilityHandleorauthorizationHandlevalues fail integrity binding.Set both artifacts to 2 MiB in the tampered branch. Assert that the same untampered pair succeeds before asserting each handle mismatch fails.
🤖 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_server_intake.rs` around lines 133 - 142, Update the tampered branch in the SCCM intake test to configure both artifacts at 2 MiB so validation reaches capabilityHandle and authorizationHandle comparisons within the byte budget. Before checking each tampered-handle failure, assert that the corresponding untampered handle pair succeeds, then retain separate assertions that each handle mismatch is rejected.
19-104: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd an observed-provenance advanced-capture fixture.
The fixture builder covers operator-declared provenance only. It does not execute the observed-provenance checks for role matching,
configuredpath state, and matchingpathClass.Add one observed artifact with its advanced role in
rolesObserved. Add rejection cases for a mismatchedpathClassand an observed advanced role with noadvancedCaptureartifact.🤖 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_server_intake.rs` around lines 19 - 104, Extend advanced_capture_manifest to support an observed-provenance fixture, including an advanced role in topology.rolesObserved and an artifact whose role and configured path provenance are observed with matching pathClass and advancedCapture metadata. Add test cases covering rejection of a mismatched pathClass and rejection when the observed advanced role has no advancedCapture artifact, while preserving the existing operator-declared fixture behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/superpowers/plans/2026-08-05-sccm-advanced-capture-main-integration.md`:
- Around line 134-143: Update the “Step 3: Run strict quality gates” checklist
so it does not claim completion while the listed full-repository cargo fmt check
fails. Either leave the checklist item unchecked or replace that command with
the scoped rustfmt command that passed, and ensure the recorded results
accurately match the commands shown.
---
Outside diff comments:
In `@crates/cmtraceopen-parser/src/sccm/server/windows/intake.rs`:
- Around line 661-680: Add Rustdoc documenting the published meanings of
SccmServerCaptureContract and every public field, including the distinction
between role and path claims and their provenance. Also document
SccmServerAdvancedProvenance and both Observed and OperatorDeclared variants
with stable API semantics.
---
Duplicate comments:
In `@crates/cmtraceopen-parser/tests/sccm_server_intake.rs`:
- Around line 133-142: Update the tampered branch in the SCCM intake test to
configure both artifacts at 2 MiB so validation reaches capabilityHandle and
authorizationHandle comparisons within the byte budget. Before checking each
tampered-handle failure, assert that the corresponding untampered handle pair
succeeds, then retain separate assertions that each handle mismatch is rejected.
- Around line 19-104: Extend advanced_capture_manifest to support an
observed-provenance fixture, including an advanced role in
topology.rolesObserved and an artifact whose role and configured path provenance
are observed with matching pathClass and advancedCapture metadata. Add test
cases covering rejection of a mismatched pathClass and rejection when the
observed advanced role has no advancedCapture artifact, while preserving the
existing operator-declared fixture behavior.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ebef49a4-c82a-4f77-b90f-1b0883b467ef
📒 Files selected for processing (5)
crates/cmtraceopen-parser/src/sccm/server/windows/intake.rscrates/cmtraceopen-parser/tests/sccm_server_intake.rsdocs/superpowers/plans/2026-08-05-sccm-advanced-capture-main-integration.mdlibrary.mdsrc-tauri/Cargo.toml
| - [x] **Step 3: Run strict quality gates** | ||
|
|
||
| ```bash | ||
| cargo fmt --all -- --check | ||
| cargo clippy --locked -p cmtraceopen-parser --all-targets -- -D warnings | ||
| cargo clippy --locked -p cmtrace-open --all-targets --all-features -- -D warnings | ||
| git diff --check | ||
| ``` | ||
|
|
||
| Result: both clippy commands, the scoped rustfmt check over the PR Rust delta, and `git diff --check` pass. Full-repository `cargo fmt --all -- --check` remains red on pre-existing out-of-scope Intune, Jamf, and elevation formatting drift; no unrelated files were reformatted for this integration. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Do not mark the strict quality gate complete while its listed command fails.
Line 137 runs full cargo fmt --all -- --check. Line 143 records that this command remains red. Leave this task unchecked, or replace the command list with the scoped command that actually passed. The current status makes the evidence pack state that a failed gate completed.
🤖 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 `@docs/superpowers/plans/2026-08-05-sccm-advanced-capture-main-integration.md`
around lines 134 - 143, Update the “Step 3: Run strict quality gates” checklist
so it does not claim completion while the listed full-repository cargo fmt check
fails. Either leave the checklist item unchecked or replace that command with
the scoped rustfmt command that passed, and ensure the recorded results
accurately match the commands shown.
Frozen main integration evidence — 2026-08-05UPDATE — Target
UPDATE — Integration decisions
UPDATE — Privacy/catalog assertions
UPDATE — Local verification
UPDATE — Exact Windows x64 artifact
ASK — Windows-only acceptance still required Run the frozen MSI on an installed SCCM server and inspect the real Registry/CIM role facts, configured roots, Windows reparse/no-follow behavior, observed-vs-operator-declared labels, single-use capability lifecycle, and bounded current/ |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src-tauri/src/sccm/collector/advanced_capture.rs`:
- Around line 1030-1036: Update the read handling in the advanced capture flow
near validate_opened_source so a short read from
input.take(read_limit).read_to_end is accepted instead of returning
CopyAdvancedFailure::Failed; retain map_copy_error handling for actual read
errors and the existing read_limit bound.
- Around line 1160-1169: Update the Windows path normalization used by
windows_path_key() so it strips the same verbatim prefixes handled by
windows_final_path(), ensuring canonicalize() results and captured paths compare
in the same form instead of being rejected as CopyAdvancedFailure::Unsafe. Keep
the existing non-Windows behavior unchanged, and add a Windows-gated regression
test that covers a canonicalize() output with a \\?\-prefixed path comparing
successfully against the final path.
In `@src-tauri/src/sccm/collector/discovery.rs`:
- Around line 518-519: Update the manifest assertion in the surrounding test to
parse the file contents as JSON and assert the deserialized manifest’s
roleProvenance field equals "observed". Replace the formatted substring check
while preserving the existing file read and failure behavior.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ba116062-1a7d-4b61-bb37-fcb2b665faca
📒 Files selected for processing (5)
docs/superpowers/plans/2026-08-05-sccm-advanced-server-validation-rework.mdlibrary.mdsrc-tauri/src/sccm/collector/advanced_capture.rssrc-tauri/src/sccm/collector/discovery.rssrc-tauri/tests/sccm_advanced_server_capture.rs
| input | ||
| .take(read_limit) | ||
| .read_to_end(&mut bytes) | ||
| .map_err(map_copy_error)?; | ||
| if bytes.len() as u64 != read_limit { | ||
| return Err(CopyAdvancedFailure::Failed); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
A concurrent truncation still turns a benign short read into a total capture failure.
opened.len is read from the handle before read_to_end. The site server appends to and rotates smspxe.log, CloudMgr.log, and BgbServer.log continuously. If the file is truncated after opened_source_facts returns and before the read completes, bytes.len() is below read_limit and line 1035 returns CopyAdvancedFailure::Failed. capture_advanced_authorized then propagates SccmCollectorError::CaptureFailed, the bundle root is removed, and the capability was already consumed by store.consume. The operator must repeat the picker and both consent prompts.
The identity checks in validate_opened_source narrow this window but do not close it. take(read_limit) already bounds growth, so accepting a short read remains safe: extra bytes can never be read.
🐛 Proposed fix
- let capped = opened.len > byte_limit;
let read_limit = opened.len.min(byte_limit);
let mut bytes =
Vec::with_capacity(usize::try_from(read_limit).map_err(|_| CopyAdvancedFailure::Failed)?);
input
.take(read_limit)
.read_to_end(&mut bytes)
.map_err(map_copy_error)?;
- if bytes.len() as u64 != read_limit {
- return Err(CopyAdvancedFailure::Failed);
- }
+ // The source can shrink between the validated open and the read. A short
+ // read is a bounded fragment, not a failure. Growth stays bounded by take().
+ let capped = opened.len > byte_limit && bytes.len() as u64 == read_limit;
Ok((bytes, capped))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src-tauri/src/sccm/collector/advanced_capture.rs` around lines 1030 - 1036,
Update the read handling in the advanced capture flow near
validate_opened_source so a short read from input.take(read_limit).read_to_end
is accepted instead of returning CopyAdvancedFailure::Failed; retain
map_copy_error handling for actual read errors and the existing read_limit
bound.
| let manifest = fs::read_to_string(bundle.join("sccm-server-manifest.json")).unwrap(); | ||
| assert!(manifest.contains("\"roleProvenance\": \"observed\"")); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Assert the parsed manifest field, not a formatted JSON substring.
Line 519 matches the literal "roleProvenance": "observed", including the space after the colon. That spacing is an artifact of pretty serialization. If the manifest writer switches to serde_json::to_string, the substring disappears and the test fails without any behavior change. If a different field ever carries the same literal text, the test passes for the wrong reason.
Parse the manifest and assert the field.
♻️ Proposed change
- let manifest = fs::read_to_string(bundle.join("sccm-server-manifest.json")).unwrap();
- assert!(manifest.contains("\"roleProvenance\": \"observed\""));
+ let manifest: serde_json::Value =
+ serde_json::from_reader(fs::File::open(bundle.join("sccm-server-manifest.json")).unwrap())
+ .unwrap();
+ let artifacts = manifest["artifacts"].as_array().unwrap();
+ assert!(!artifacts.is_empty());
+ assert!(artifacts
+ .iter()
+ .all(|artifact| artifact["roleProvenance"] == "observed"));📝 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.
| let manifest = fs::read_to_string(bundle.join("sccm-server-manifest.json")).unwrap(); | |
| assert!(manifest.contains("\"roleProvenance\": \"observed\"")); | |
| let manifest: serde_json::Value = | |
| serde_json::from_reader(fs::File::open(bundle.join("sccm-server-manifest.json")).unwrap()) | |
| .unwrap(); | |
| let artifacts = manifest["artifacts"].as_array().unwrap(); | |
| assert!(!artifacts.is_empty()); | |
| assert!(artifacts | |
| .iter() | |
| .all(|artifact| artifact["roleProvenance"] == "observed")); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src-tauri/src/sccm/collector/discovery.rs` around lines 518 - 519, Update the
manifest assertion in the surrounding test to parse the file contents as JSON
and assert the deserialized manifest’s roleProvenance field equals "observed".
Replace the formatted substring check while preserving the existing file read
and failure behavior.
Server-validation rework evidenceUPDATE — Target
UPDATE — Claims inspected
UPDATE — Reproduce / verification
UPDATE — Windows artifact provenance
DECISION — Scope boundary No new speculative advanced-role inference was added. Sources without genuinely observed native role/root/PXE evidence remain operator-declared candidates rather than receiving manufactured observed provenance. ASK — Critic gate Please re-review the frozen target and exact hosted artifact above. No raw SCCM/server evidence is included in this comment. Out of scope: merging PR #500 or publishing raw environment evidence. |
Configured-root provenance rework evidenceUPDATE — Target
UPDATE — Claims inspected
UPDATE — Reproduce / verification
UPDATE — Windows artifact provenance
DECISION — Scope boundary Inferred/site/default roots remain usable for normal native collection but can never self-promote BGB to observed. Only a successfully read trusted MP ASK — Critic gate Please re-review the frozen target and exact hosted artifact above. Out of scope: merging PR #500 or publishing raw SCCM/server evidence. |
Windows lab merge gate — blockedIndependent code review: ACCEPT at Remaining mandatory Setup-CM lab evidence:
Disposition: keep PR open; do not merge or close #497 until this checklist is satisfied on the exact artifact. |
PR #500 MSI policy diagnosis — BLOCKED
Blocker classification: Sanitized blocker: The shell was not elevated, so the exact machine MSI install returned Narrowest lab-safe remediation: rerun the exact MSI from its local NTFS source in an elevated administrator lab session; do not change installer policy or uninstall the existing product. |
|
Sanitized MSI policy diagnosis for commit
|
…server-capture # Conflicts: # library.md
There was a problem hiding this comment.
Pull request overview
Adds an end-to-end, bounded “advanced SCCM server capture” flow that lets an operator authorize a directory once (via picker + consent), receive a short-lived single-use opaque capability, and then perform a tightly allowlisted capture of six specific server logs while keeping operator-declared provenance distinct from observed provenance. This extends across the React workspace UI, typed IPC commands, a non-persistent in-memory capability store, a bounded native collector that writes a provenance-safe manifest, and parser intake validation/gating.
Changes:
- Frontend: renders advanced source options, runs picker + consent, and performs authorize/capture/cancel with store-backed phase handling.
- Backend: adds capability issuance/consumption/cancel commands + AppState store, and implements bounded collector + discovery provenance assembly for advanced sources.
- Parser: extends server intake schema to validate and integrity-bind
captureContract, enforce per-source capped budgets, and keep advanced artifacts parser-ineligible.
Reviewed changes
Copilot reviewed 25 out of 26 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/workspaces/sccm/types.ts | Adds advanced source option / authorization request / capability DTOs and expanded SCCM role union. |
| src/workspaces/sccm/SccmWorkspace.tsx | Implements picker + consent UI and wires advanced authorize/capture/cancel flow. |
| src/workspaces/sccm/SccmWorkspace.test.tsx | Adds UI tests covering advanced flow, cancellation on decline/unmount/replacement. |
| src/workspaces/sccm/sccm-workspace.css | Styles the new “advanced sources” panel and status indicator. |
| src/workspaces/sccm/sccm-store.ts | Extends Zustand state machine with authorizing / capturingAdvanced and capability storage. |
| src/workspaces/sccm/sccm-store.test.ts | Adds store behavior test for advanced capability lifecycle clearing on failure. |
| src/lib/commands.ts | Adds typed IPC wrappers for advanced authorization/capture/cancel commands. |
| src/lib/commands.test.ts | Verifies IPC boundary stays capability-only (no raw path returned / reused). |
| src-tauri/tests/sccm_native_collection.rs | Updates fixtures to include capture-root provenance origin. |
| src-tauri/tests/sccm_advanced_server_capture.rs | New native tests covering allowlist contract, bounded capture, provenance, and hostile-file defenses. |
| src-tauri/tests/sccm_advanced_ipc.rs | New tests for DTO strictness, capability lifecycle (TTL/capacity/cancel), and symlink-root rejection. |
| src-tauri/src/state/app_state.rs | Adds sccm_advanced_capabilities store to AppState under sccm-diagnostics. |
| src-tauri/src/sccm/collector/mod.rs | Exposes advanced capture module/types; extends discovery DTO and root provenance origin tracking. |
| src-tauri/src/sccm/collector/discovery.rs | Assembles native environment with controlled advanced facts and publishes sanitized advanced options. |
| src-tauri/src/sccm/collector/advanced_capture.rs | New advanced collector owner: allowlist contracts, capability store, bounded copy, manifest emission. |
| src-tauri/src/lib.rs | Registers new SCCM advanced IPC commands behind sccm-diagnostics. |
| src-tauri/src/commands/sccm.rs | Adds Tauri commands and helpers for authorize/capture/cancel backed by AppState capability store. |
| src-tauri/Cargo.toml | Adds getrandom dependency and registers new SCCM advanced integration tests. |
| library.md | Adds links to SCCM advanced capture integration/validation plans. |
| docs/superpowers/plans/2026-08-05-sccm-bgb-configured-root-provenance.md | New plan doc for BGB configured-root provenance gating. |
| docs/superpowers/plans/2026-08-05-sccm-advanced-server-validation-rework.md | New plan doc for server-validation gaps and verification steps. |
| docs/superpowers/plans/2026-08-05-sccm-advanced-capture-main-integration.md | New plan doc for merging advanced capture work onto main and verifying boundaries. |
| docs/superpowers/plans/2026-08-04-sccm-advanced-server-capture.md | New frozen implementation plan/spec for advanced server capture feature. |
| crates/cmtraceopen-parser/tests/sccm_server_intake.rs | Adds parser tests for advanced-capture contract validation, provenance, and budget enforcement. |
| crates/cmtraceopen-parser/src/sccm/server/windows/intake.rs | Extends intake schema to support/validate captureContract and enforce advanced-capture constraints. |
| Cargo.lock | Updates lockfile for the new dependency. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| #[tauri::command] | ||
| pub fn authorize_sccm_advanced_capture( | ||
| state: State<'_, AppState>, | ||
| request: SccmAdvancedCaptureAuthorizationRequest, | ||
| ) -> Result<SccmAdvancedCaptureCapability, AppError> { | ||
| let mut store = state | ||
| .sccm_advanced_capabilities | ||
| .lock() | ||
| .map_err(|_| collector_error(SccmCollectorError::AdvancedAuthorizationRejected))?; | ||
| authorize_advanced_with_provider( | ||
| &NativeDiscoveryProvider, | ||
| &mut store, | ||
| request, | ||
| Instant::now(), | ||
| ) | ||
| } |
| fn capture_contract_identity(contract: Option<&SccmServerCaptureContract>) -> String { | ||
| contract.map_or_else(String::new, |contract| { | ||
| format!( | ||
| "{}\0{}\0{}\0{}\0{}\0{}\0{:?}\0{:?}", | ||
| contract.card_id, | ||
| contract.card_version, | ||
| contract.capability_handle, | ||
| contract.authorization_handle, | ||
| contract.role_claim, | ||
| contract.path_claim, | ||
| contract.role_provenance, | ||
| contract.path_provenance, | ||
| ) | ||
| }) | ||
| } |
…under Unreleased (#530) Catch up the Unreleased section with all ten commits merged since the last changelog update (54d539e / #513): - Microsoft Store app evidence lane (#358 / #518) - Reducer Framework v1 governance, ADRs, and charters (#519) - Windows Autopilot evidence parser outside ESP (#362 / #450) - Company Portal Windows LocalState logs (#366 / #460) - Bounded advanced SCCM server capture (#500), folded into the existing native SCCM diagnostics path bullet - Agent tooling / Clairvoyance staff org scaffolding (#516) - Dependency bumps (quick-xml, time, install-action) and the GitHub Sponsors funding link No version bump: package.json/Cargo.toml/tauri.conf.json remain at 1.5.1 with no new tag, so this stays purely an Unreleased catch-up. Claude-Session: https://claude.ai/code/session_01A8z5Ysfa5Afts6gVPmHQZj Co-authored-by: Claude <noreply@anthropic.com>
Closes #497. Supports evidence capture for #475, #476, #477, #478, and #479 without promoting their semantic source cards.
Adds one closed six-source SCCM advanced capture surface across the operator UI, typed IPC, single-use native capability store, bounded collector, manifest writer, and parser intake. Observed and operator-declared provenance remain distinct; operator candidates stay unclassified, parser-ineligible, and non-semantic. Capabilities are random, opaque, expiring, bounded, consumed before read, canceled on abandonment, and nonpersistent. Each logical source is limited to two current/lo_ rotations and 4 MiB total, enforced by both collector and hostile-manifest parser gates.
Independent rework and current-main integration critics: ACCEPT at final SHA d31ae86.
Local gates: full parser/app suites; Rust 1.88; WASM; focused intake, IPC, collector, hierarchy, database, client provenance, and native collection suites; frontend tests; TypeScript; production build; scoped rustfmt and diff checks.
Windows hosted Registry/CIM, reparse-point behavior, real picker/consent states, and fresh SCCM lab capture remain mandatory before merge. No raw lab evidence will be uploaded.
Summary by CodeRabbit