fix(sccm): capture client logs beside CcmExec - #494
Conversation
github-actions
Bot
added
bug
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe change derives the SCCM client log root from the exact ChangesSCCM client capture
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CIMQuery
participant Discovery
participant ClientCollector
participant Manifest
CIMQuery->>Discovery: Return CcmExec Name and PathName
Discovery->>Discovery: Validate executable path
Discovery->>ClientCollector: Provide derived Logs root
ClientCollector->>Manifest: Record capture results without source paths
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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-04-sccm-client-service-root-capture.md`:
- Line 5: Replace the en dash between the issue references in the Goal line with
plain text such as “#483 through `#485`,” preserving the rest of the documentation
unchanged.
- Line 15: Update the “Task 1: Parse the exact CcmExec service path” heading to
level 2 so it follows the level-1 document title and satisfies the heading
hierarchy rule.
In `@src-tauri/src/sccm/collector/discovery.rs`:
- Around line 309-313: Update client_root_from_service_path to reject any
path_name whose original value differs from path_name.trim(), alongside the
existing control-character validation, then parse the original untrimmed value
rather than normalizing it. Add test cases covering leading and trailing
ordinary spaces next to the control-whitespace cases.
In `@src-tauri/tests/sccm_native_collection.rs`:
- Around line 352-353: Update the assertions in the artifact verification test
around artifact.content_sha256 to compare the digest value against the known
SHA-256 of content, rather than only checking that it is present. Preserve the
existing bytes_copied assertion and ensure the test fails for incorrect or
constant digests.
🪄 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: aea083e5-5501-4752-b8e3-755d390795a7
📒 Files selected for processing (3)
docs/superpowers/plans/2026-08-04-sccm-client-service-root-capture.mdsrc-tauri/src/sccm/collector/discovery.rssrc-tauri/tests/sccm_native_collection.rs
|
|
||
| > **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. | ||
|
|
||
| **Goal:** Capture evidence for #483–#485 from the exact client log root beside a validated `CcmExec.exe`, without exposing paths or weakening catalog/cap/reparse controls. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Replace the en dash.
Line 5 uses an en dash between issue references. Replace it with text such as #483 through #485``.
As per path instructions, "**/*.md: Documentation. Verify commands, paths, and flags actually exist in the repo. Flag em dashes and en dashes."
🤖 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-04-sccm-client-service-root-capture.md` at
line 5, Replace the en dash between the issue references in the Goal line with
plain text such as “#483 through `#485`,” preserving the rest of the documentation
unchanged.
Source: Path instructions
|
|
||
| --- | ||
|
|
||
| ### Task 1: Parse the exact CcmExec service path |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the heading hierarchy.
Line 15 starts at heading level 3 after the level-1 document title. Change ### Task 1 to ## Task 1 to satisfy MD001.
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 15-15: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
🤖 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-04-sccm-client-service-root-capture.md` at
line 15, Update the “Task 1: Parse the exact CcmExec service path” heading to
level 2 so it follows the level-1 document title and satisfies the heading
hierarchy rule.
Source: Linters/SAST tools
| fn client_root_from_service_path(path_name: &str) -> Option<std::path::PathBuf> { | ||
| if path_name.chars().any(char::is_control) { | ||
| return None; | ||
| } | ||
| let path_name = path_name.trim(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject whitespace normalization in service paths.
Line 313 accepts leading or trailing ordinary spaces by trimming before validation. For example, a service value with a leading space becomes an accepted drive-qualified path. Reject values where path_name != path_name.trim() and parse the original value. Add plain-space cases beside the control-whitespace test.
Proposed fix
- let path_name = path_name.trim();
- if path_name.is_empty() {
+ if path_name.is_empty() || path_name != path_name.trim() {
return None;
}📝 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.
| fn client_root_from_service_path(path_name: &str) -> Option<std::path::PathBuf> { | |
| if path_name.chars().any(char::is_control) { | |
| return None; | |
| } | |
| let path_name = path_name.trim(); | |
| fn client_root_from_service_path(path_name: &str) -> Option<std::path::PathBuf> { | |
| if path_name.chars().any(char::is_control) { | |
| return None; | |
| } | |
| if path_name.is_empty() || path_name != path_name.trim() { | |
| return None; | |
| } |
🤖 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 309 - 313, Update
client_root_from_service_path to reject any path_name whose original value
differs from path_name.trim(), alongside the existing control-character
validation, then parse the original untrimmed value rather than normalizing it.
Add test cases covering leading and trailing ordinary spaces next to the
control-whitespace cases.
| assert_eq!(artifact.bytes_copied, content.len() as u64); | ||
| assert!(artifact.content_sha256.is_some()); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Verify the digest value.
Line 353 only verifies that content_sha256 exists. A constant or incorrect digest passes this test. Compare it with the known SHA-256 of content.
As per path instructions, "src-tauri/tests/**/*.rs: Regression tests. 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 `@src-tauri/tests/sccm_native_collection.rs` around lines 352 - 353, Update the
assertions in the artifact verification test around artifact.content_sha256 to
compare the digest value against the known SHA-256 of content, rather than only
checking that it is present. Preserve the existing bytes_copied assertion and
ensure the test fails for incorrect or constant digests.
Source: Path instructions
|
Final integration review ACCEPT at 11fc0f7. The delta is one cfg line: the byte-wrapper is test-only, while Windows runtime discovery still parses CIM output through apply_cim_service_facts and client_root_from_service_facts. Focused discovery tests and strict native Clippy pass. Hosted Windows release build is the remaining merge gate. |
1 participant
Summary
CcmExec.exeservice path%WINDIR%\\CCM\\Logsfallbackmtrmgr.logunsupported until observed evidence existsValidation
e33b436aindependently ACCEPTEDSupports #483, #484, and #485. Those discovery issues remain open pending a fresh Windows capture and evidence audit.
Summary by CodeRabbit
New Features
CcmExecservice configuration.Bug Fixes
mtrmgr.logfrom collection.