feat(sccm): enforce evidence-backed findings - #353
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 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 public SCCM finding contract with typed phases, confidence, evidence, coverage gaps, correlation keys, terminal evidence, and bounded artifact requests. Findings support builder construction, validated serialization, normalization, deterministic ordering, corroboration checks, and expanded contract tests. ChangesSCCM finding contracts
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant SccmFindingBuilder
participant SccmFinding
participant EvidenceValidation
participant WireDeserializer
SccmFindingBuilder->>SccmFinding: build finding
SccmFinding->>EvidenceValidation: validate evidence, roles, keys, and requests
EvidenceValidation-->>SccmFinding: return validation result
WireDeserializer->>SccmFinding: deserialize validated wire fields
Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Comment |
|
@coderabbitai review Please perform a substantive review of exact head Focus on conservative soundness: an empty parser-owned Stable-profile registry must keep arbitrary/unregistered profile strings from authorizing key-only High; duplicate/same-time/keyless/Low/unprofiled/mismatched/uncited evidence must fail closed; terminal markers must be cited and non-forgeable through unknown wire values; InsufficientEvidence must retain explicit non-Captured gaps and exact catalog logical-ID/role requests with bounded reasons/count; drive/root/glob/recursive/whole-disk requests must fail; and custom deserialization must sort/dedup then revalidate while rejecting unknown fields/raw execution context. Please also check deterministic ordering, camelCase round trips, public compatibility, and pure wasm32 scope. This PR remains draft pending CodeRabbit and a different independent API reviewer. |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
base="25b37333affde22b4ef8a19f4a5f3d89c082b599"
head="8612c172a827fb663d6572074dcf9b2c08c91519"
echo "== Checked-out commit =="
git rev-parse HEAD
echo "== Base/head availability =="
git cat-file -t "$base"
git cat-file -t "$head"
echo "== Changed files (exact range) =="
git diff --name-status "$base" "$head"
echo "== Diff stat =="
git diff --stat "$base" "$head"
echo "== Relevant symbols and source map =="
fd -t f -e rs . | rg '(^|/)(findings|sccm_spine_contract)\.rs$' || true
fd -t f -e rs . | xargs -r ast-grep outline --items all --type struct,enum,impl,function --match 'Sccm|Finding|Artifact|Coverage|Terminal|Confidence|Phase' 2>/dev/null || true
echo "== Exact diff =="
git diff --find-renames --find-copies --no-ext-diff "$base" "$head"Length of output: 58157 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== SCCM module source files =="
fd -t f -e rs . crates/cmtraceopen-parser/src/sccm | sort
echo "== Model and catalog declarations used by findings =="
ast-grep outline crates/cmtraceopen-parser/src/sccm/models.rs --items all --type struct,enum,impl,function --match 'SccmEvidenceRef|SccmCorrelationKey|SccmRole|SccmCoverageState|SccmFindingClass|SccmKeyConfidence'
ast-grep outline crates/cmtraceopen-parser/src/sccm/catalog.rs --items all --type struct,enum,impl,function --match 'declared_source_catalog|SccmSource|Catalog'
echo "== Exact source sections =="
rg -n -A45 -B8 'struct SccmEvidenceRef|struct SccmCorrelationKey|enum SccmRole|enum SccmCoverageState|enum SccmFindingClass|enum SccmKeyConfidence' crates/cmtraceopen-parser/src/sccm/models.rs
rg -n -A120 -B8 'fn declared_source_catalog|struct Sccm.*Catalog|logical_name' crates/cmtraceopen-parser/src/sccm/catalog.rs
echo "== All construction and validation/serde entry points =="
rg -n -C4 -P '\bSccmFinding\s*\{' crates/cmtraceopen-parser
rg -n -C3 -P '\bSccmFindingBuilder::new\s*\(|\b\.validate\s*\(\)|from_(str|slice|value)\s*::<\s*SccmFinding\s*>' crates/cmtraceopen-parser
echo "== Catalog logical IDs and any duplicate logical-id/role pairs =="
rg -n -P 'logical_name:\s*"|role:\s*SccmRole::' crates/cmtraceopen-parser/src/sccm/catalog.rs
echo "== Dependency/scope indicators in the exact diff =="
git diff --no-ext-diff 25b37333affde22b4ef8a19f4a5f3d89c082b599 8612c172a827fb663d6572074dcf9b2c08c91519 -- \
'Cargo.toml' 'Cargo.lock' \
| sed -n '1,240p'
rg -n -i -C2 'std::fs|std::net|std::process|tauri|windows|winapi|ParserKind|LogEntry|native' crates/cmtraceopen-parser/src/sccm/findings.rs crates/cmtraceopen-parser/src/sccm/mod.rsLength of output: 2594 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== Model and catalog maps =="
ast-grep outline crates/cmtraceopen-parser/src/sccm/models.rs --items all --type struct,enum,function 2>/dev/null || true
ast-grep outline crates/cmtraceopen-parser/src/sccm/catalog.rs --items all --type struct,enum,function 2>/dev/null || true
echo "== Model declarations used by findings =="
rg -n -A55 -B6 '^(pub )?(struct|enum) (SccmEvidenceRef|SccmCorrelationKey|SccmRole|SccmCoverageState|SccmFindingClass|SccmKeyConfidence)\b' crates/cmtraceopen-parser/src/sccm/models.rs || true
echo "== Catalog declaration and entries =="
rg -n -A160 -B8 '^(pub )?fn declared_source_catalog\b|logical_name:' crates/cmtraceopen-parser/src/sccm/catalog.rs || true
echo "== Direct construction and contract entry points =="
rg -n -C4 -P '\bSccmFinding\s*\{' crates/cmtraceopen-parser || true
rg -n -C3 -P '\bSccmFindingBuilder::new\s*\(|\b\.validate\s*\(\)|from_(str|slice|value)\s*::<\s*SccmFinding\s*>' crates/cmtraceopen-parser || true
echo "== Dependency changes in exact range =="
git diff --no-ext-diff 25b37333affde22b4ef8a19f4a5f3d89c082b599 8612c172a827fb663d6572074dcf9b2c08c91519 -- Cargo.toml Cargo.lock || true
echo "== Native/I-O/prohibited scope indicators in changed implementation =="
rg -n -i -C2 'std::fs|std::net|std::process|tauri|windows|winapi|ParserKind|LogEntry|native' \
crates/cmtraceopen-parser/src/sccm/findings.rs \
crates/cmtraceopen-parser/src/sccm/mod.rs || trueLength of output: 55756 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== Strict-wire structs and nested fields (numbered) =="
nl -ba crates/cmtraceopen-parser/src/sccm/findings.rs | sed -n '110,220p'
echo
nl -ba crates/cmtraceopen-parser/src/sccm/findings.rs | sed -n '445,575p'
echo "== Deserialization/adversarial-test coverage (numbered) =="
nl -ba crates/cmtraceopen-parser/tests/sccm_spine_contract.rs | sed -n '535,785p'
echo "== Every serde unknown-field policy in SCCM models/findings =="
rg -n -C2 'deny_unknown_fields|derive\(.*Deserialize|Deserialize\)\]' \
crates/cmtraceopen-parser/src/sccm/findings.rs \
crates/cmtraceopen-parser/src/sccm/models.rs
echo "== Check exact base/head integrity and uncommitted state =="
git diff --quiet 25b37333affde22b4ef8a19f4a5f3d89c082b599 8612c172a827fb663d6572074dcf9b2c08c91519 -- \
crates/cmtraceopen-parser/src/sccm/findings.rs \
crates/cmtraceopen-parser/src/sccm/mod.rs \
crates/cmtraceopen-parser/tests/sccm_spine_contract.rs
printf 'exact-range status: %s\n' "$?"
git status --shortLength of output: 1420 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== Strict-wire structs and nested fields (numbered) =="
cat -n crates/cmtraceopen-parser/src/sccm/findings.rs | sed -n '110,220p'
echo
cat -n crates/cmtraceopen-parser/src/sccm/findings.rs | sed -n '445,575p'
echo "== Deserialization/adversarial-test coverage (numbered) =="
cat -n crates/cmtraceopen-parser/tests/sccm_spine_contract.rs | sed -n '535,785p'
echo "== Every serde unknown-field policy in SCCM models/findings =="
rg -n -C2 'deny_unknown_fields|derive\(.*Deserialize|Deserialize\)\]' \
crates/cmtraceopen-parser/src/sccm/findings.rs \
crates/cmtraceopen-parser/src/sccm/models.rs || true
echo "== Exact range and working tree =="
if git diff --quiet 25b37333affde22b4ef8a19f4a5f3d89c082b599 8612c172a827fb663d6572074dcf9b2c08c91519 -- \
crates/cmtraceopen-parser/src/sccm/findings.rs \
crates/cmtraceopen-parser/src/sccm/mod.rs \
crates/cmtraceopen-parser/tests/sccm_spine_contract.rs; then
echo "unexpected: exact range has no changes"
else
echo "exact range contains the reviewed SCCM changes"
fi
git status --short || trueLength of output: 29575
Finding
I also confirmed the requested core safeguards are otherwise present: the empty parser-owned Stable-profile registry prevents arbitrary profile strings from authorizing key-only High; cited observed terminal failures are required for High ✅ Action performedReview 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/findings.rs (2)
15-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the intentionally empty stable-profile registry.
REGISTERED_STABLE_CORRELATION_PROFILE_IDSbeing empty makeshas_profiled_key_corroborationalways returnfalse, so the corroboration branch invalidate(Line 250) is currently unreachable. That is the correct fail-closed default, but a short comment stating the intent (and what must be true before an ID is added) prevents someone from relaxing it accidentally.♻️ Suggested comment
+/// Intentionally empty: no correlation profile has yet been validated as stable +/// enough to authorize key-only High-confidence ConfirmedFailure findings. +/// Adding an ID here weakens the evidence bar and requires contract review. const REGISTERED_STABLE_CORRELATION_PROFILE_IDS: &[&str] = &[];🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/cmtraceopen-parser/src/sccm/findings.rs` around lines 15 - 18, Document the intentional fail-closed behavior beside REGISTERED_STABLE_CORRELATION_PROFILE_IDS, noting that it must remain empty until a stable correlation profile ID is explicitly verified and registered; do not change the registry or validation logic.
452-478: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueAvoid rebuilding the whole declared catalog on every validation.
declared_source_catalog()allocates aVecwith aStringper catalog entry (percrates/cmtraceopen-parser/src/sccm/catalog.rs:431-439) each timevalidate()runs, then does a linear scan per request. Once findings are validated in bulk by a reducer this becomes a needless hot-path allocation. Consider a borrowed/static lookup keyed by logical name, or hoisting the catalog fetch outside the loop-per-finding call path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/cmtraceopen-parser/src/sccm/findings.rs` around lines 452 - 478, Update validate_artifact_requests to avoid calling declared_source_catalog() and allocating catalog entries for every validation; reuse a borrowed or static logical-name lookup, or hoist the catalog construction outside the per-finding validation path. Preserve the existing request-count, reason, declaration, and role-mismatch checks while using the shared catalog representation for each request.crates/cmtraceopen-parser/tests/sccm_spine_contract.rs (2)
760-776: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest only proves top-level strictness despite its name.
Injecting
executionContextat the root is rejected bydeny_unknown_fieldsonSccmFindingWire, but the same key nested underevidence[0],terminalEvidence[0].reference, orcorrelationKeys[0]is silently ignored today. Extend this test with a nested injection case so it actually guards the claim once nested wire types become strict.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/cmtraceopen-parser/tests/sccm_spine_contract.rs` around lines 760 - 776, Extend finding_deserialization_rejects_raw_execution_context_fields with a nested executionContext injection under evidence[0], terminalEvidence[0].reference, or correlationKeys[0], and assert deserialization fails for that payload. Preserve the existing top-level assertion while ensuring the test guards strict rejection of raw execution-context fields in nested wire structures.
163-225: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCorroboration acceptance path has no positive test.
Because
REGISTERED_STABLE_CORRELATION_PROFILE_IDSis empty, every one of these cases fails for the same reason (no registered profile), sohas_profiled_key_corroboration's distinct-identity/kind/normalized logic is never exercised in the accepting direction. A silent regression there (e.g.>= 2becoming>= 1) would not be caught. Consider exposing a test-only registered profile ID or unit-testing the predicate directly so both outcomes are pinned.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/cmtraceopen-parser/tests/sccm_spine_contract.rs` around lines 163 - 225, The corroboration tests only cover rejection because no registered profile exists, leaving has_profiled_key_corroboration’s accepting path untested. Add a test-only registered correlation profile or directly unit-test has_profiled_key_corroboration with two distinct, same-kind, normalized matching keys, and also preserve a rejection case for duplicate identity/key evidence so the required corroboration threshold remains enforced.
🤖 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/findings.rs`:
- Around line 534-570: Replace the nested scan in has_profiled_key_corroboration
with a single-pass grouping approach keyed by kind, normalized value, and
extraction profile, tracking distinct evidence identities per group. Ensure the
implementation returns true once any group has at least two identities, while
preserving the existing corroborating-key and missing-evidence filtering
behavior; do not rely on linear contains scans.
- Around line 428-437: Update validate_required_text and the SccmFindingBuilder
construction/validation flow to reject SccmPhase::Unknown values that shadow
declared phase names, matching Serialize for SccmPhase behavior. Ensure
SccmFindingBuilder::phase returns the validation error during construction while
preserving existing required-field checks.
---
Nitpick comments:
In `@crates/cmtraceopen-parser/src/sccm/findings.rs`:
- Around line 15-18: Document the intentional fail-closed behavior beside
REGISTERED_STABLE_CORRELATION_PROFILE_IDS, noting that it must remain empty
until a stable correlation profile ID is explicitly verified and registered; do
not change the registry or validation logic.
- Around line 452-478: Update validate_artifact_requests to avoid calling
declared_source_catalog() and allocating catalog entries for every validation;
reuse a borrowed or static logical-name lookup, or hoist the catalog
construction outside the per-finding validation path. Preserve the existing
request-count, reason, declaration, and role-mismatch checks while using the
shared catalog representation for each request.
In `@crates/cmtraceopen-parser/tests/sccm_spine_contract.rs`:
- Around line 760-776: Extend
finding_deserialization_rejects_raw_execution_context_fields with a nested
executionContext injection under evidence[0], terminalEvidence[0].reference, or
correlationKeys[0], and assert deserialization fails for that payload. Preserve
the existing top-level assertion while ensuring the test guards strict rejection
of raw execution-context fields in nested wire structures.
- Around line 163-225: The corroboration tests only cover rejection because no
registered profile exists, leaving has_profiled_key_corroboration’s accepting
path untested. Add a test-only registered correlation profile or directly
unit-test has_profiled_key_corroboration with two distinct, same-kind,
normalized matching keys, and also preserve a rejection case for duplicate
identity/key evidence so the required corroboration threshold remains enforced.
🪄 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: e3c1e6f8-c272-43b5-b7af-71c497b766f2
📒 Files selected for processing (3)
crates/cmtraceopen-parser/src/sccm/findings.rscrates/cmtraceopen-parser/src/sccm/mod.rscrates/cmtraceopen-parser/tests/sccm_spine_contract.rs
|
@coderabbitai review Please perform a fresh substantive review of exact head Prior review disposition:
Re-check conservative evidence rules, recursive wire strictness, phase construction/serialization consistency, deterministic normalization, public model compatibility, and pure Rust/wasm32 scope. Verification at this head: 23 focused finding tests; 677 parser tests on Rust 1.88; strict Clippy; Rust 1.88 wasm32 check; TypeScript; scoped Rust 1.88 formatting; and diff checks all pass. The PR remains draft pending this pass and a different independent API reviewer. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
base="25b37333affde22b4ef8a19f4a5f3d89c082b599"
head="5fd80ee9f97aa986708f9d8ea27da14111934602"
echo "== Exact revisions =="
printf 'checked out: '; git rev-parse HEAD
printf 'base object: '; git cat-file -t "$base"
printf 'head object: '; git cat-file -t "$head"
echo "== Changed files =="
git diff --name-status "$base" "$head"
echo "== Diff stat =="
git diff --stat "$base" "$head"
echo "== Relevant source/test files =="
fd -t f -e rs . | rg '(^|/)(findings|models|catalog|sccm_spine_contract)\.rs$' | sort
echo "== SCCM declaration map =="
ast-grep outline crates/cmtraceopen-parser/src/sccm --items all \
--type struct,enum,impl,function --match 'Sccm|Finding|Artifact|Coverage|Terminal|Confidence|Phase' 2>/dev/null || true
echo "== Exact diff =="
git diff --no-ext-diff --find-renames --find-copies "$base" "$head" -- \
crates/cmtraceopen-parser/src/sccm \
crates/cmtraceopen-parser/tests/sccm_spine_contract.rs \
Cargo.toml Cargo.lockLength of output: 65856 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== Dependent SCCM contract declarations =="
rg -n -A70 -B6 \
'^(pub )?(struct|enum) (SccmEvidenceRef|SccmCorrelationKey|SccmCorrelationKeyKind|SccmCoverageState|SccmFindingClass|SccmKeyConfidence|SccmRole)\b' \
crates/cmtraceopen-parser/src/sccm/models.rs
echo "== Catalog source and logical-ID/role entries =="
ast-grep outline crates/cmtraceopen-parser/src/sccm/catalog.rs --items all --type struct,enum,function 2>/dev/null || true
rg -n -A180 -B10 '^(pub )?fn declared_source_catalog\b|logical_name:|role:' \
crates/cmtraceopen-parser/src/sccm/catalog.rs
echo "== Finding implementation tail and all changed test names =="
cat -n crates/cmtraceopen-parser/src/sccm/findings.rs | sed -n '280,851p'
rg -n '^\s*fn finding_|^\s*#\[test\]' crates/cmtraceopen-parser/tests/sccm_spine_contract.rs
echo "== Finding construction, deserialization, and public-export usages =="
rg -n -C3 -P '\bSccmFinding(?:Builder)?\b|from_(str|slice|value)\s*::<\s*SccmFinding\s*>' \
crates/cmtraceopen-parser
echo "== Changed-scope indicators =="
rg -n -i -C2 'std::fs|std::net|std::process|tauri|windows|winapi|tokio|reqwest|unsafe' \
crates/cmtraceopen-parser/src/sccm/findings.rs \
crates/cmtraceopen-parser/src/sccm/mod.rs \
crates/cmtraceopen-parser/tests/sccm_spine_contract.rs || true
echo "== Workspace and exact range status =="
git status --short
git diff --check 25b37333affde22b4ef8a19f4a5f3d89c082b599 5fd80ee9f97aa986708f9d8ea27da14111934602Length of output: 92031
Finding
The previously reported fixes are present: nested finding wire values now reject unknown fields recursively at all seven tested insertion points, and The diff remains limited to parser SCCM contracts and tests, with no dependency, native I/O, Windows/Tauri, or wasm32-scope additions. You are interacting with an AI system. |
|
@coderabbitai review Please perform a fresh substantive review of exact head The latest P1 from #353 (comment) is fixed test-first:
Please re-check every evidence/coverage/key/request mutation path, valid round trips and field shape/order, recursive strict deserialization, phase-shadow validation, deterministic behavior, empty Stable-profile fail-closed behavior, and wasm32 purity. Verification on this exact head: 24 focused finding tests; 678 parser tests on Rust 1.88; strict Clippy; Rust 1.88 wasm32; TypeScript; scoped Rust 1.88 formatting; and diff checks all pass. PR remains draft pending this review and a different independent API reviewer. |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
base="25b37333affde22b4ef8a19f4a5f3d89c082b599"
head="dddcd03ecb532875981d90d71a181d0c06115c1a"
echo "== Exact revisions =="
printf 'checked out: '; git rev-parse HEAD
printf 'base: '; git cat-file -t "$base"
printf 'head: '; git cat-file -t "$head"
echo "== Changed files and diff stat =="
git diff --name-status "$base" "$head"
git diff --stat "$base" "$head"
echo "== SCCM source map =="
ast-grep outline crates/cmtraceopen-parser/src/sccm --items all \
--type struct,enum,impl,function --match 'Sccm|Finding|Artifact|Coverage|Terminal|Confidence|Phase' 2>/dev/null || true
echo "== Exact implementation diff =="
git diff --no-ext-diff --find-renames --find-copies "$base" "$head" -- \
crates/cmtraceopen-parser/src/sccm \
crates/cmtraceopen-parser/tests/sccm_spine_contract.rs \
Cargo.toml Cargo.lock
echo "== Findings source, numbered =="
cat -n crates/cmtraceopen-parser/src/sccm/findings.rs
echo "== Dependent model and catalog contracts =="
rg -n -A85 -B8 \
'^(pub )?(struct|enum) (SccmEvidenceRef|SccmCorrelationKey|SccmCorrelationKeyKind|SccmCoverageState|SccmFindingClass|SccmKeyConfidence|SccmRole)\b' \
crates/cmtraceopen-parser/src/sccm/models.rs || true
rg -n -A190 -B10 '^(pub )?fn declared_source_catalog\b|logical_name:|role:' \
crates/cmtraceopen-parser/src/sccm/catalog.rs || true
echo "== Findings tests and serde/mutation test contexts =="
rg -n -A35 -B8 \
'finding_|serializ|deserializ|unknown|executionContext|phase|determin|correlation|artifact|coverage|terminal' \
crates/cmtraceopen-parser/tests/sccm_spine_contract.rs || true
echo "== All public construction/serialization/deserialization paths =="
rg -n -C4 -P \
'\bSccmFinding(?:Builder)?\b|serde_json::(?:to_|from_)|from_(?:str|slice|value)\s*::<\s*SccmFinding\s*>' \
crates/cmtraceopen-parser || true
echo "== Scope/dependency indicators and whitespace =="
git diff --no-ext-diff "$base" "$head" -- Cargo.toml Cargo.lock
rg -n -i -C2 'std::fs|std::net|std::process|tauri|windows|winapi|tokio|reqwest|unsafe' \
crates/cmtraceopen-parser/src/sccm/findings.rs \
crates/cmtraceopen-parser/src/sccm/mod.rs \
crates/cmtraceopen-parser/tests/sccm_spine_contract.rs || true
git diff --check "$base" "$head"Length of output: 197544 🏁 Script executed: #!/bin/bash
set -euo pipefail
base="25b37333affde22b4ef8a19f4a5f3d89c082b599"
head="dddcd03ecb532875981d90d71a181d0c06115c1a"
echo "== Latest serializer-related delta only =="
git diff --no-ext-diff 5fd80ee9f97aa986708f9d8ea27da14111934602 "$head" -- \
crates/cmtraceopen-parser/src/sccm/findings.rs \
crates/cmtraceopen-parser/tests/sccm_spine_contract.rs
echo "== Finding wire types, serializer, and deserializer =="
sed -n '1,370p' crates/cmtraceopen-parser/src/sccm/findings.rs | cat -n
echo "== Validator, builder, helpers, and normalization =="
sed -n '367,850p' crates/cmtraceopen-parser/src/sccm/findings.rs | cat -n
echo "== Finding-only contract tests =="
sed -n '1,1010p' crates/cmtraceopen-parser/tests/sccm_spine_contract.rs | cat -n
echo "== Relevant model definitions exactly =="
rg -n -A95 -B5 \
'^(pub )?(struct|enum) (SccmEvidenceRef|SccmCorrelationKey|SccmCorrelationKeyKind|SccmCoverageState|SccmFindingClass|SccmKeyConfidence|SccmRole)\b' \
crates/cmtraceopen-parser/src/sccm/models.rs
echo "== Serialize implementations of field-member types =="
rg -n -A25 -B5 'impl Serialize for|derive\(.*Serialize' \
crates/cmtraceopen-parser/src/sccm/findings.rs \
crates/cmtraceopen-parser/src/sccm/models.rs
echo "== Exact-range changed paths and dependency diff =="
git diff --name-status "$base" "$head"
git diff --no-ext-diff "$base" "$head" -- Cargo.toml Cargo.lockLength of output: 126680
Finding
The reported serializer fix is otherwise correctly present:
✅ Action performedReview finished.
|
|
@coderabbitai review Please perform a fresh substantive review of exact head The evidence-identity P1 from #353 (comment) is fixed test-first:
Please re-check self-attestation, nested refs, error precedence, valid Verification on this exact head: 30 focused finding tests; 684 parser tests on Rust 1.88; strict Clippy; Rust 1.88 wasm32; TypeScript; scoped Rust 1.88 formatting; and diff checks all pass. PR remains draft pending this pass and a different independent API reviewer. |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
base="25b37333affde22b4ef8a19f4a5f3d89c082b599"
head="e69eb1c073e912d1a65f8ce0debd4e904b0639cb"
echo "== Exact revisions =="
printf 'checked out: '; git rev-parse HEAD
printf 'base type: '; git cat-file -t "$base"
printf 'head type: '; git cat-file -t "$head"
echo "== Changed paths and stat =="
git diff --name-status "$base" "$head"
git diff --stat "$base" "$head"
echo "== SCCM API map at requested head =="
git show "$head:crates/cmtraceopen-parser/src/sccm/findings.rs" > /tmp/findings.rs
ast-grep outline /tmp/findings.rs --items all --type struct,enum,impl,function \
--match 'Sccm|Finding|Evidence|Artifact|Coverage|Terminal|Confidence|Phase' || true
echo "== Exact finding implementation: wire types, serializer, deserializer, validator and normalization =="
git show "$head:crates/cmtraceopen-parser/src/sccm/findings.rs" | cat -n | sed -n '1,920p'
echo "== Dependent public evidence/key/role model declarations =="
git show "$head:crates/cmtraceopen-parser/src/sccm/models.rs" | \
rg -n -A100 -B8 \
'^(pub )?(struct|enum) (SccmEvidenceRef|SccmCorrelationKey|SccmCorrelationKeyKind|SccmCoverageState|SccmFindingClass|SccmKeyConfidence|SccmRole)\b' || true
echo "== Catalog logical source declarations =="
git show "$head:crates/cmtraceopen-parser/src/sccm/catalog.rs" | \
rg -n -A190 -B10 '^(pub )?fn declared_source_catalog\b|logical_name:|role:' || true
echo "== Finding-focused tests at requested head =="
git show "$head:crates/cmtraceopen-parser/tests/sccm_spine_contract.rs" | cat -n | \
rg -n -A42 -B10 \
'finding_|evidence.*ref|reference|serializ|deserializ|unknown|executionContext|phase|determin|correlation|artifact|coverage|terminal' || true
echo "== Exact full diff for reviewed files =="
git diff --no-ext-diff --find-renames --find-copies "$base" "$head" -- \
crates/cmtraceopen-parser/src/sccm \
crates/cmtraceopen-parser/tests/sccm_spine_contract.rs \
Cargo.toml Cargo.lock
echo "== Scope and dependency indicators in exact range =="
git diff --no-ext-diff "$base" "$head" -- Cargo.toml Cargo.lock
git diff --no-ext-diff "$base" "$head" -- \
crates/cmtraceopen-parser/src/sccm/findings.rs \
crates/cmtraceopen-parser/src/sccm/mod.rs \
crates/cmtraceopen-parser/tests/sccm_spine_contract.rs | \
rg -n -i 'std::fs|std::net|std::process|tauri|windows|winapi|tokio|reqwest|unsafe' || true
echo "== Whitespace validation =="
git diff --check "$base" "$head"Length of output: 93710 🏁 Script executed: #!/bin/bash
set -euo pipefail
base="25b37333affde22b4ef8a19f4a5f3d89c082b599"
prior="dddcd03ecb532875981d90d71a181d0c06115c1a"
head="e69eb1c073e912d1a65f8ce0debd4e904b0639cb"
echo "== Latest-only delta (prior reviewed head -> requested head) =="
git diff --no-ext-diff "$prior" "$head" -- \
crates/cmtraceopen-parser/src/sccm/findings.rs \
crates/cmtraceopen-parser/tests/sccm_spine_contract.rs
echo "== Exact dependent model definitions and serde implementations =="
git show "$head:crates/cmtraceopen-parser/src/sccm/models.rs" > /tmp/sccm-models.rs
ast-grep outline /tmp/sccm-models.rs --items all --type struct,enum,impl,function \
--match 'SccmEvidenceRef|SccmCorrelationKey|SccmRole|SccmCoverageState|SccmFindingClass|SccmKeyConfidence' || true
git show "$head:crates/cmtraceopen-parser/src/sccm/models.rs" | cat -n | \
sed -n '/pub struct SccmEvidenceRef/,/pub struct SccmEvidence/p; /pub struct SccmCorrelationKey/,/pub struct Sccm/p; /pub enum SccmRole/,/pub enum Sccm/p; /pub enum SccmCoverageState/,/pub enum Sccm/p; /pub enum SccmFindingClass/,/pub enum Sccm/p; /pub enum SccmKeyConfidence/,/pub enum Sccm/p' || true
echo "== All exact-head finding references and serde boundaries =="
git grep -n -E \
'SccmFinding(::|[[:space:]]*\{)|SccmFindingBuilder::new|from_(str|slice|value).*SccmFinding|to_(string|value).*SccmFinding|serialize.*SccmFinding' \
"$head" -- crates/cmtraceopen-parser || true
echo "== Validator ordering / normalize and direct serialize tests, concise =="
git show "$head:crates/cmtraceopen-parser/src/sccm/findings.rs" | cat -n | sed -n '150,620p'
git show "$head:crates/cmtraceopen-parser/tests/sccm_spine_contract.rs" | cat -n | \
sed -n '620,1180p'
echo "== Unknown-field policy and direct derived serde on public nested types =="
git show "$head:crates/cmtraceopen-parser/src/sccm/findings.rs" | \
rg -n -C2 'deny_unknown_fields|impl (Serialize|Deserialize) for|derive\(.*(Serialize|Deserialize)' || true
git show "$head:crates/cmtraceopen-parser/src/sccm/models.rs" | \
rg -n -C2 'SccmEvidenceRef|SccmCorrelationKey|derive\(.*(Serialize|Deserialize)|impl (Serialize|Deserialize) for' || true
echo "== Exact diff check and changed files outside declared scope =="
git diff --check "$base" "$head"
git diff --name-only "$base" "$head" | sortLength of output: 73717
No blocking findings. Confirmed in this pass:
✅ Action performedReview finished.
|
adamgell
left a comment
adamgell
left a comment
There was a problem hiding this comment.
Independent API/security review of exact range 25b37333affde22b4ef8a19f4a5f3d89c082b599..e69eb1c073e912d1a65f8ce0debd4e904b0639cb: BLOCKED.
CodeRabbit completed a substantive review on this exact head and reported no blocking findings (comment). A separate dynamic builder/deserializer/serializer matrix found these blocking contract gaps:
-
Valid post-build mutation bypasses canonical output.
SccmFindingexposes mutable public arrays. Reversingevidenceand appending a duplicate after construction still serializes successfully as[B, A, B]. The custom serializer validates current state, but neither canonicalizes it nor rejects a valid-but-noncanonical state. The evidence-first deterministic-output contract therefore does not hold for the public type; the same exposure applies to the other canonicalized arrays. -
Logical evidence identities can be duplicated or aliased. Two refs with the same
artifact_id + entry_idbut different line ranges both survive normalization, even though that same pair is the logical identity used for corroboration. Leading/trailing whitespace in both IDs is also accepted (for example" artifact-a "/" entry-a "). Exact-structdedup()plus nonblank-only validation does not provide a unique canonical evidence identity. -
Unknown-value shadows break canonical/round-trip semantics.
SccmPhase::Unknown(" policy ")bypasses the known-phase shadow guard.SccmRole::Unknown("client")serializes as"client"and deserializes asSccmRole::Client, so a valid finding does not round-trip to itself. The role behavior originates in the shared model but is admitted by this new public finding API and needs a guard or shared-model correction before the contract is public. -
The bounded-request guard is too literal. All of these were accepted by the builder: “Collect every file on the system.”, “Scan the full disk for related evidence.”, “Collect the complete C: drive.”, “Walk all directories under C:.”, and “Collect drive-wide logs.” This does not enforce the plan's prohibition on requests for an unbounded entire drive.
Independent committed focused tests are green: cargo test --locked -p cmtraceopen-parser --test sccm_spine_contract finding_ -- --nocapture → 30 passed. The temporary adversarial probe was 3 passed / 6 failed and has been removed; it exercised post-build mutation, duplicate/whitespace evidence identities, phase/role shadows, nested range validation, all non-captured coverage states, and unbounded-request variants.
Please add focused regression tests for each case and make construction, deserialization, serialization, and round-trip behavior converge on one deterministic invariant. This review does not authorize merge, ready-for-review, or issue closure.
|
@coderabbitai review Please review the exact range This head includes the independent blocker matrix and the two accepted local CodeRabbit follow-ups. Please pay particular attention to deterministic post-mutation serialization, canonical evidence identity/range conflicts across all reference surfaces, |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
base="25b37333affde22b4ef8a19f4a5f3d89c082b599"
head="310cee519e68900e6a77dc7025d86b383154dc08"
printf 'checked-out: '; git rev-parse HEAD
printf 'base: '; git cat-file -t "$base"
printf 'head: '; git cat-file -t "$head"
echo "== Changed paths/stat =="
git diff --name-status "$base" "$head"
git diff --stat "$base" "$head"
echo "== SCCM source/test map =="
fd -t f -e rs . crates/cmtraceopen-parser/src/sccm crates/cmtraceopen-parser/tests | sort
ast-grep outline crates/cmtraceopen-parser/src/sccm --items all \
--type struct,enum,impl,function \
--match 'Sccm|Finding|Evidence|Terminal|Artifact|Coverage|Correlation|Phase|Role' 2>/dev/null || true
echo "== Exact diff =="
git diff --no-ext-diff --find-renames --find-copies "$base" "$head" -- \
crates/cmtraceopen-parser/src/sccm \
crates/cmtraceopen-parser/tests/sccm_spine_contract.rs \
Cargo.toml Cargo.lock
echo "== Findings implementation (numbered) =="
git show "$head:crates/cmtraceopen-parser/src/sccm/findings.rs" | cat -n
echo "== Dependent public models/catalog (numbered excerpts) =="
git show "$head:crates/cmtraceopen-parser/src/sccm/models.rs" | cat -n | \
rg -n -A110 -B8 \
'pub (struct|enum) (SccmEvidenceRef|SccmCorrelationKey|SccmCorrelationKeyKind|SccmRole|SccmCoverageState|SccmFindingClass|SccmKeyConfidence)' || true
git show "$head:crates/cmtraceopen-parser/src/sccm/catalog.rs" | cat -n | \
rg -n -A190 -B8 'fn declared_source_catalog|logical_name:|role:' || true
echo "== Finding-focused tests (numbered) =="
git show "$head:crates/cmtraceopen-parser/tests/sccm_spine_contract.rs" | cat -n | \
rg -n -A60 -B10 \
'finding_|serializ|deserializ|mutation|reference|range|unknown|phase|role|artifact|request|coverage|terminal|determin|canonical' || true
echo "== Public construction/serde references =="
git grep -n -E \
'SccmFinding(::|[[:space:]]*\{)|SccmFindingBuilder::new|from_(str|slice|value).*SccmFinding|to_(string|value).*SccmFinding' \
"$head" -- crates/cmtraceopen-parser || true
echo "== Scope/dependency/whitespace =="
git diff --no-ext-diff "$base" "$head" -- Cargo.toml Cargo.lock
git diff --no-ext-diff "$base" "$head" -- \
crates/cmtraceopen-parser/src/sccm \
crates/cmtraceopen-parser/tests/sccm_spine_contract.rs | \
rg -n -i 'std::fs|std::net|std::process|tauri|windows|winapi|tokio|reqwest|unsafe' || true
git diff --check "$base" "$head"Length of output: 132027 🏁 Script executed: #!/bin/bash
set -euo pipefail
head="310cee519e68900e6a77dc7025d86b383154dc08"
echo "== findings.rs: public wires, serde, and validation =="
git show "$head:crates/cmtraceopen-parser/src/sccm/findings.rs" | nl -ba | sed -n '1,760p'
echo "== findings.rs: remaining helpers/builder =="
git show "$head:crates/cmtraceopen-parser/src/sccm/findings.rs" | nl -ba | sed -n '760,980p'
echo "== Blocker-matrix tests: deterministic/canonical reference behavior =="
git show "$head:crates/cmtraceopen-parser/tests/sccm_spine_contract.rs" | nl -ba | sed -n '430,940p'
echo "== Request wording tests and helpers =="
git show "$head:crates/cmtraceopen-parser/tests/sccm_spine_contract.rs" | nl -ba | sed -n '40,120p; 900,1020p'
echo "== Phase/role definitions and serde implementations =="
git show "$head:crates/cmtraceopen-parser/src/sccm/findings.rs" | nl -ba | sed -n '1,160p'
git show "$head:crates/cmtraceopen-parser/src/sccm/models.rs" | nl -ba | \
rg -n -A140 -B10 'enum SccmRole|impl SccmRole|enum SccmCoverageState|struct SccmEvidenceRef|struct SccmCorrelationKey'
echo "== Latest delta only, for accepted follow-ups =="
git diff --no-ext-diff e69eb1c073e912d1a65f8ce0debd4e904b0639cb "$head" -- \
crates/cmtraceopen-parser/src/sccm/findings.rs \
crates/cmtraceopen-parser/tests/sccm_spine_contract.rsLength of output: 1710 🏁 Script executed: #!/bin/bash
set -euo pipefail
head="310cee519e68900e6a77dc7025d86b383154dc08"
echo "== findings.rs: public wires, serde, and validation =="
git show "$head:crates/cmtraceopen-parser/src/sccm/findings.rs" | cat -n | sed -n '1,760p'
echo "== findings.rs: remaining helpers/builder =="
git show "$head:crates/cmtraceopen-parser/src/sccm/findings.rs" | cat -n | sed -n '760,980p'
echo "== Blocker-matrix tests: deterministic/canonical reference behavior =="
git show "$head:crates/cmtraceopen-parser/tests/sccm_spine_contract.rs" | cat -n | sed -n '430,940p'
echo "== Request wording tests and helpers =="
git show "$head:crates/cmtraceopen-parser/tests/sccm_spine_contract.rs" | cat -n | sed -n '40,120p; 900,1020p'
echo "== Phase/role definitions and serde implementations =="
git show "$head:crates/cmtraceopen-parser/src/sccm/findings.rs" | cat -n | sed -n '1,160p'
git show "$head:crates/cmtraceopen-parser/src/sccm/models.rs" | cat -n | \
rg -n -A140 -B10 'enum SccmRole|impl SccmRole|enum SccmCoverageState|struct SccmEvidenceRef|struct SccmCorrelationKey'
echo "== Latest delta only, for accepted follow-ups =="
git diff --no-ext-diff e69eb1c073e912d1a65f8ce0debd4e904b0639cb "$head" -- \
crates/cmtraceopen-parser/src/sccm/findings.rs \
crates/cmtraceopen-parser/tests/sccm_spine_contract.rsLength of output: 117714
Finding
The requested safeguards otherwise hold in this pass:
✅ Action performedReview finished.
|
There was a problem hiding this comment.
🧹 Nitpick comments (3)
crates/cmtraceopen-parser/tests/sccm_spine_contract.rs (3)
746-758: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the exact error variant here.
Every neighbouring builder test compares against a specific
SccmFindingValidationError; this one only checksis_err(), so it would still pass if the whitespace-wrapped phase were rejected for an unrelated reason.♻️ Suggested tightening
- assert!(result.is_err()); + assert_eq!( + result.unwrap_err(), + SccmFindingValidationError::MissingRequiredField + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/cmtraceopen-parser/tests/sccm_spine_contract.rs` around lines 746 - 758, Update finding_rejects_whitespace_wrapped_declared_phase_shadow to assert the specific SccmFindingValidationError returned by SccmFindingBuilder::build, matching the neighboring builder tests and verifying the rejection is caused by the whitespace-wrapped phase.
847-895: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest name promises ordering that isn't asserted.
finding_validates_finding_gap_and_request_roles_before_other_rulesonly asserts thatInvalidRoleis returned. Each fixture is otherwise valid, soInvalidRoleis the only possible error and precedence over the other rules is never exercised. Either rename to drop the ordering claim, or add a fixture that violates a second rule (e.g. anInsufficientEvidencefinding with no coverage gap plus an invalid role) to proveInvalidRolewins.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/cmtraceopen-parser/tests/sccm_spine_contract.rs` around lines 847 - 895, Update the test `finding_validates_finding_gap_and_request_roles_before_other_rules` so it actually exercises validation precedence by making at least one fixture violate both `InvalidRole` and another rule, such as insufficient evidence without a coverage gap, and assert that `InvalidRole` is returned first; alternatively, rename the test to remove the ordering claim if precedence is not intended to be tested.
946-976: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBare
is_err()plus missing filesystem-root cases.Two gaps in this table test:
assert!(result.is_err())doesn't pin the failure toInvalidArtifactRequestReason; a regression that instead tripsUndeclaredArtifactRequestorArtifactRequestRoleMismatchwould keep this green.- The reason list covers prose forms ("filesystem root", "under C:.") but no literal root path such as
C:\,\\server\share, or/. That is exactly the outstanding P1 called out in the PR discussion, so adding these rows will make the fix verifiable.♻️ Suggested additions
"Collect from the filesystem root.", "Use a glob for matching log files.", r"Collect C:\Windows\CCM\Logs\*.log.", + r"Collect C:\.", + r"Collect \\LAB-CLIENT-01\C$.", + "Collect /.", ]; for reason in reasons { @@ - assert!(result.is_err(), "{reason}"); + assert_eq!( + result.unwrap_err(), + SccmFindingValidationError::InvalidArtifactRequestReason, + "{reason}" + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/cmtraceopen-parser/tests/sccm_spine_contract.rs` around lines 946 - 976, Strengthen finding_artifact_requests_reject_structurally_unbounded_reasons by matching the build error specifically to InvalidArtifactRequestReason rather than using a bare is_err assertion, while preserving the existing request setup. Expand its reasons table with literal filesystem-root cases such as C:\, \\server\share, and /, and verify each is rejected for that exact error variant.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/cmtraceopen-parser/tests/sccm_spine_contract.rs`:
- Around line 746-758: Update
finding_rejects_whitespace_wrapped_declared_phase_shadow to assert the specific
SccmFindingValidationError returned by SccmFindingBuilder::build, matching the
neighboring builder tests and verifying the rejection is caused by the
whitespace-wrapped phase.
- Around line 847-895: Update the test
`finding_validates_finding_gap_and_request_roles_before_other_rules` so it
actually exercises validation precedence by making at least one fixture violate
both `InvalidRole` and another rule, such as insufficient evidence without a
coverage gap, and assert that `InvalidRole` is returned first; alternatively,
rename the test to remove the ordering claim if precedence is not intended to be
tested.
- Around line 946-976: Strengthen
finding_artifact_requests_reject_structurally_unbounded_reasons by matching the
build error specifically to InvalidArtifactRequestReason rather than using a
bare is_err assertion, while preserving the existing request setup. Expand its
reasons table with literal filesystem-root cases such as C:\, \\server\share,
and /, and verify each is rejected for that exact error variant.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 764dbf07-1965-4840-808d-eb61649fb52e
📒 Files selected for processing (3)
crates/cmtraceopen-parser/src/sccm/findings.rscrates/cmtraceopen-parser/src/sccm/models.rscrates/cmtraceopen-parser/tests/sccm_spine_contract.rs
|
@coderabbitai review Please review exact range Disposition of the prior exact-head P1: accepted and fixed test-first in Please keep PR #353 draft pending this exact-head review and a different independent API review. |
adamgell
left a comment
adamgell
left a comment
There was a problem hiding this comment.
Independent exact-head API/security rereview of 25b37333affde22b4ef8a19f4a5f3d89c082b599..670fe2d5bf84f2aae3b8dd78a5895a74501e3b71: BLOCKED.
The new rooted-path fix works for Windows drive/absolute, POSIX absolute/root, and UNC forms. The full dynamic boundary/equivalence replay still finds three blocking contract classes:
-
Public validation boundaries disagree.
- A post-build
SccmPhase::Unknown(" policy ")is accepted by directvalidate(), while builder construction and finding serialization reject it. Standalone phase serialization also admits the whitespace shadow. - Same-range whitespace aliases of a cited evidence identity on each of top-level evidence, terminal evidence, and correlation-key evidence are rejected by direct
validate(), while builder construction and serialization normalize and accept the same logical identity. - A whitespace-wrapped catalog logical ID/reason has the same split: direct
validate()rejects it, while builder and serializer normalize and accept it.
The public contract needs one invariant at direct validation, construction, deserialization, and serialization. Canonical aliases may be normalized or rejected, but the result cannot depend on which boundary the caller uses.
- A post-build
-
The request-count invariant is bypassed by serialization normalization. Seventeen identical requests are rejected by builder construction, direct
validate(), and deserialization because the raw count exceeds 16; the same post-build mutation serializes successfully after deduplication to one request. Decide whether the cap applies before or after canonicalization, then enforce that choice at every boundary. -
Free-text scope matching has both false negatives and false positives.
- Still accepted as unbounded:
allfiles,everydirectory,entiredisk,wholefilesystem, and “Collect every log on the system.” - Incorrectly rejected despite requesting one exact catalog artifact: “Collect the full disk imaging Task Sequence log.”, “Confirm the whole-disk encryption status recorded in PolicyAgent.log.”, “Confirm the system-wide assignment recorded in PolicyAgent.log.”, “Confirm recursive retry behavior recorded in PolicyAgent.log.”, and “Confirm all files were downloaded, as recorded in PolicyAgent.log.”
The guard must distinguish requested collection scope from words describing the bounded workflow/evidence. Add both negative equivalence rows and positive disk-imaging/workflow controls so tightening one side cannot regress the other.
- Still accepted as unbounded:
Independent results on this exact SHA:
- committed focused finding tests: 46 passed
- temporary adversarial replay: 5 passed / 6 failed
- confirmed green in the replay: canonical post-build collection serialization, conflicting range rejection across build/validate/serde, standalone/finding/gap/request role shadows plus canonical future role, all recursive unknown-wire insertions, and terminal/key/confidence/all-six-gap rules.
CodeRabbit performed a substantive review of the prior exact 310cee5 and found the rooted-path P1, which 670fe2d fixes. The exact-670fe2d CodeRabbit request is pending; the authenticated local CLI was also attempted but hit the service's recoverable rate limit. A green status alone is not being treated as substantive approval.
Please commit focused red/green regressions for these cases and request another exact-SHA CodeRabbit and independent review. This review does not authorize merge, ready-for-review, or issue closure.
|
Ownership claim for the blocked exact-head correction: |
|
@coderabbitai review Please perform a substantive review of exact range Disposition of independent blocker review
The authenticated local CLI was also attempted on the final uncommitted delta and returned a recoverable 13-minute rate limit; that is not being represented as approval. Keep PR #353 draft pending this exact-head review and a different independent API rereview. |
adamgell
left a comment
adamgell
left a comment
There was a problem hiding this comment.
Independent exact-head API/security review — BLOCKED
Reviewed exact range 25b37333affde22b4ef8a19f4a5f3d89c082b599..a515f33858be599759dd84e02353bc1f92f4a16f in a fresh detached read-only worktree. The PR head, fetched remote ref, and checkout all resolve to a515f33858be599759dd84e02353bc1f92f4a16f; the review worktree is clean. This is not approval. Keep PR #353 draft and open.
[P1] Active has/have confirmation forms still authorize broad targets
has_passive_unbounded_confirmation_request() only enters its broad-target check for tokens recognized by is_passive_auxiliary(). However, had, has, and have are accepted evidence predicates but are not passive auxiliaries. The later broad-scope check also treats files as bounded merely because a state-observation token and catalog identity occur elsewhere in the clause.
A disposable external consumer probe reproduced four active forms as accepted by the builder, direct validate(), validating serialization, and strict deserialization — 16/16 unsafe accepts:
Confirm PolicyAgent.log status has all files provided.Confirm PolicyAgent.log status has every file provided.Confirm all files have provided status in PolicyAgent.log.Confirm Smsts.log imaging status has the full disk provided.
These are the same broad-target requirement hidden behind accepted active evidence vocabulary. Add permanent four-boundary regressions and make the quantified broad-target rule independent of passive versus active auxiliary selection. Preserve the existing bounded observations such as Confirm all files were downloaded, as recorded in PolicyAgent.log.
[P1] Public evidence redaction remains message-path-specific and field-incomplete
The direct Windows identity matcher drops a match whenever the preceding byte is a path separator, and the fallback path matcher only recognizes the literal Users directory. The sensitive-label grammar also omits common structured Windows identity fields. A deterministic normalized-evidence probe showed the raw identity in both SccmEvidence.message and serialized JSON for all six cases — 12 public-surface leaks:
C:\Profiles\LAB\SyntheticUser\profile.dat\\server\home\LAB\SyntheticUser\cacheC:\Profiles\.\LocalUser\profile.dat{"domain":"LAB","accountName":"SyntheticJsonUser"}{"domain":"LAB","samAccountName":"SyntheticSamUser"}localUser=LocalStructuredUser
UPN redaction remained green, and the committed safe path/code controls remained green. Separately, SccmRawEvidenceSnapshot::export() copies component and ccm_source_file verbatim. A normalized CCM record with component="LAB\ComponentUser" and file="LAB\FileUser" exposed both raw identities in public SccmEvidence JSON. The spine contract says raw user names or user context must never reach public evidence; projection cannot be limited to the message field or one directory spelling. Add deterministic evidence-object and JSON regressions while preserving genuinely safe path fragments and diagnostic codes.
[P1] Coverage-only states still become diagnostic claims
SccmFinding::validate() accepts any nonempty coverage-gap vector as a substitute for evidence, then applies terminal/corroboration guards only to High ConfirmedFailure and High LikelyContributor. A disposable four-boundary probe built coverage-only Symptom/High, LikelyContributor/Moderate, and ConfirmedFailure/Moderate findings from AccessDenied alone — 12/12 builder/validate/serialize/deserialize accepts.
That directly violates the program constraint that absent, access-denied, capped, skipped, unsupported, malformed, or partial sources are coverage states, not proof of a symptom, contributor, or failure. Reserve coverage-only output for InsufficientEvidence with its bounded next-artifact request, or require cited evidence for every diagnostic claim class.
Verified closures, gates, and corroboration
- The prior key/profile blocker is closed at this SHA: five missing/unknown/experimental/forged/malformed Strong or Exact profile cases were rejected at all four public finding boundaries. The registered stable-profile set remains empty and fail-closed; conservative Low controls remain green.
- Committed suites are green: finding contracts 71/71, complete spine 133/133, full parser 726/726.
- Repository-default strict Clippy passed. Rust 1.88 strict Clippy reports the same 59 unrelated
uninlined_format_argsbaseline findings at exact base and head, with none in the five changed files. - Rust 1.88 wasm32 check passed; TypeScript 7.0.2
tsc --noEmitpassed; scoped Rust 1.88 formatting and base/head plus incrementalgit diff --checkpassed. - Full-workspace Rust 1.88 formatting still reports the same unrelated 17-file baseline outside this range.
- No dependency change, native/Windows/Tauri/network/database/process I/O,
ParserKind::Sccm, or publicLogEntrychange was found. - Hosted CodeRabbit completed a substantive exact-head review with no blocker: #353 (comment). The authenticated local CodeRabbit 0.7.1 run returned status-only output, so it is not claimed as another approval. The reproduced failures above supersede the hosted no-blocker result.
Do not mark ready, merge, resolve ownership/history threads, close #318, or unblock downstream production reducers/correlation. Unblock after permanent tests and corrections for all three contract classes, every prescribed gate, fresh substantive CodeRabbit on the corrected exact SHA, and a different independent exact-head PASS.
Exact-head review-fix round — implemented, still draftThe independent API/security review on
Fresh verification on exact head
The plan-prescribed repository-wide Remaining review blockers: a fresh substantive hosted CodeRabbit review and a different independent exact-head review of |
|
@coderabbitai review Please perform a fresh substantive review of exact head This head supersedes the prior no-blocker review on Please anchor the review to exact SHA |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
base="25b37333affde22b4ef8a19f4a5f3d89c082b599"
head="5c0fda213686fd9ddb12a29723c601f675836567"
prior="a515f33858be599759dd84e02353bc1f92f4a16f"
echo "== Exact revision objects =="
printf 'checked-out: '; git rev-parse HEAD
printf 'base: '; git cat-file -t "$base"
printf 'head: '; git cat-file -t "$head"
echo "== Changed paths, stat, and whitespace =="
git diff --name-status "$base" "$head"
git diff --stat "$base" "$head"
git diff --check "$base" "$head"
echo "== Latest correction delta =="
git diff --no-ext-diff "$prior" "$head" -- \
crates/cmtraceopen-parser/src/sccm/findings.rs \
crates/cmtraceopen-parser/src/sccm/evidence.rs \
crates/cmtraceopen-parser/tests/sccm_spine_contract.rs
echo "== Declaration map for the exact head =="
git show "$head:crates/cmtraceopen-parser/src/sccm/findings.rs" > /tmp/findings-5c0fda2.rs
ast-grep outline /tmp/findings-5c0fda2.rs --items all --type struct,enum,impl,function \
--match 'Sccm|Finding|Artifact|Confirm|Evidence|Coverage|Correlation|Confidence|Phase|Terminal' || true
echo "== Finding contract, validation, serde, and normalization =="
git show "$head:crates/cmtraceopen-parser/src/sccm/findings.rs" | cat -n | \
sed -n '150,760p; 1940,2300p'
echo "== Artifact request confirmation grammar and scope controls =="
git show "$head:crates/cmtraceopen-parser/src/sccm/findings.rs" | cat -n | \
sed -n '700,1945p'
echo "== Evidence projection and redaction implementation =="
git show "$head:crates/cmtraceopen-parser/src/sccm/evidence.rs" | cat -n | \
sed -n '1,340p'
echo "== Exact regression and adjacent control tests =="
git show "$head:crates/cmtraceopen-parser/tests/sccm_spine_contract.rs" | cat -n | \
rg -n -A180 -B28 \
'all files|full disk|had|has|have|downloaded|Confirm|redact|profile|accountName|samAccountName|localUser|component|ccmSourceFile|coverage.only|InsufficientEvidence|AccessDenied|serialize|deserializ|determin' || true
echo "== Wire strictness, coverage validation, and projection call sites =="
git show "$head:crates/cmtraceopen-parser/src/sccm/findings.rs" | \
rg -n -C5 \
'deny_unknown_fields|Wire|Deserialize|Serialize|coverage|InsufficientEvidence|evidence\.is_empty|project|normalize|sort|dedup'
git show "$head:crates/cmtraceopen-parser/src/sccm/evidence.rs" | \
rg -n -C5 \
'project|redact|component|ccm|account|profile|identity|message'
echo "== Models, keys, catalog identities, and exported SCCM module =="
git show "$head:crates/cmtraceopen-parser/src/sccm/models.rs" | cat -n | \
rg -n -A115 -B12 'SccmCorrelationKey|SccmKeyConfidence|SccmEvidenceRef|SccmRole|Serialize|Deserialize' || true
git show "$head:crates/cmtraceopen-parser/src/sccm/keys.rs" | \
rg -n -C5 'normalize|profile|stable|confidence|SccmKeyConfidence' || true
git show "$head:crates/cmtraceopen-parser/src/sccm/catalog.rs" | \
rg -n -C3 'logical_name:|basename:|MP_|smsts' || true
git show "$head:crates/cmtraceopen-parser/src/sccm/mod.rs" | cat -n
echo "== Scope and public compatibility delta =="
git diff --no-ext-diff "$base" "$head" -- Cargo.toml Cargo.lock
git diff --no-ext-diff "$base" "$head" -- crates/cmtraceopen-parser/src | \
rg -n -C3 '^\+.*(LogEntry|ParserKind|std::fs|std::net|std::process|tauri|windows|winapi|tokio|reqwest|unsafe)|^\-.*(LogEntry|ParserKind)' || true
git diff --name-only "$base" "$head" | sortLength of output: 101963 🏁 Script executed: #!/bin/bash
set -euo pipefail
head="5c0fda213686fd9ddb12a29723c601f675836567"
echo "== Finding validation class and confidence rules =="
git show "$head:crates/cmtraceopen-parser/src/sccm/findings.rs" | cat -n | \
sed -n '400,700p; 1960,2140p'
echo "== Complete Confirm grammar and broad-target checks =="
git show "$head:crates/cmtraceopen-parser/src/sccm/findings.rs" | cat -n | \
sed -n '1120,1625p'
echo "== Request tokenization and suffix/actionless controls =="
git show "$head:crates/cmtraceopen-parser/src/sccm/findings.rs" | cat -n | \
sed -n '820,1125p; 1625,1960p'
echo "== Exact latest regression diff =="
git diff --no-ext-diff a515f33858be599759dd84e02353bc1f92f4a16f "$head" -- \
crates/cmtraceopen-parser/src/sccm/findings.rs \
crates/cmtraceopen-parser/src/sccm/evidence.rs \
crates/cmtraceopen-parser/tests/sccm_spine_contract.rs
echo "== Focused broad-Confirm and coverage-only test bodies =="
git show "$head:crates/cmtraceopen-parser/tests/sccm_spine_contract.rs" | \
rg -n -A150 -B30 \
'active.*broad|broad.*confirm|had|has|have|all files|full disk|coverage.only|coverage.*claim|AccessDenied|InsufficientEvidence|MissingEvidenceOrCoverageGap'
echo "== Focused projection/redaction test bodies =="
git show "$head:crates/cmtraceopen-parser/tests/sccm_spine_contract.rs" | \
rg -n -A180 -B30 \
'accountName|samAccountName|localUser|ccmSourceFile|component|profile|home|DOMAIN|redact|safe path|diagnostic code|raw snapshot'
echo "== Every parser kind and LogEntry change in the exact range =="
git diff --no-ext-diff 25b37333affde22b4ef8a19f4a5f3d89c082b599 "$head" -- \
crates/cmtraceopen-parser/src crates/cmtraceopen-parser/tests | \
rg -n -C4 'ParserKind|LogEntry|std::fs|std::net|std::process|tauri|windows|winapi|tokio|reqwest|unsafe' || true
echo "== Test names near the new contract coverage =="
git show "$head:crates/cmtraceopen-parser/tests/sccm_spine_contract.rs" | \
rg -n '^\s*fn (finding_|sccm_|public_|.*redact|.*coverage|.*confirm)' || trueLength of output: 108487 🏁 Script executed: #!/bin/bash
set -euo pipefail
head="5c0fda213686fd9ddb12a29723c601f675836567"
echo "== Passive Confirm target guard =="
git show "$head:crates/cmtraceopen-parser/src/sccm/findings.rs" | \
rg -n -A120 -B25 \
'fn has_passive_unbounded_confirmation_request|fn confirmation_clause_is_non_authorizing|fn is_confirmation_|had|has|have'
echo "== Passive Confirm regression matrix =="
git show "$head:crates/cmtraceopen-parser/tests/sccm_spine_contract.rs" | \
sed -n '2285,2355p'
echo "== Latest active-verb correction =="
git diff --no-ext-diff a515f33858be599759dd84e02353bc1f92f4a16f "$head" -- \
crates/cmtraceopen-parser/src/sccm/findings.rs \
crates/cmtraceopen-parser/tests/sccm_spine_contract.rs | \
rg -n -A100 -B20 'had|has|have|all files|full disk|Confirm'
echo "== Structured-label redaction and range handling =="
git show "$head:crates/cmtraceopen-parser/src/sccm/evidence.rs" | \
sed -n '10,195p'
git show "$head:crates/cmtraceopen-parser/src/sccm/evidence.rs" | \
rg -n -A80 -B12 'fn sensitive_value_end|fn redact_sensitive_segments|fn redact_windows_identities'
echo "== Public structured-surface regression =="
git show "$head:crates/cmtraceopen-parser/tests/sccm_spine_contract.rs" | \
sed -n '4750,4850p'
echo "== Recursive wire rejection and deterministic finding output tests =="
git show "$head:crates/cmtraceopen-parser/tests/sccm_spine_contract.rs" | \
sed -n '3190,3418p'Length of output: 115626
Finding
I also confirmed these controls:
Keep You are interacting with an AI system. |
adamgell
left a comment
adamgell
left a comment
There was a problem hiding this comment.
Independent API/security review of exact range 25b37333affde22b4ef8a19f4a5f3d89c082b599..5c0fda213686fd9ddb12a29723c601f675836567: BLOCKED. The remote PR head was re-read and matched this SHA before submission.
[P1] Auxiliary-led broad targets still pass every public boundary
has_passive_unbounded_confirmation_request() detects the broad target at crates/cmtraceopen-parser/src/sccm/findings.rs:1280-1285, but the return predicate at Lines 1290-1304 rejects had / has / have and the other non-modal auxiliaries only when a later token is one of the listed collection participles. A confirmation with the requested identity and a state observation can therefore authorize the unbound broad target directly after the auxiliary.
A disposable external-consumer probe reproduced 48 improper acceptances: 12 variants x builder, direct SccmFinding::validate(), validating serializer, and strict deserializer. The accepted variants include:
Confirm PolicyAgent.log status has all files.- the same shape with
had,have,are,be,been,being,is,was, andwere Confirm Smsts.log imaging status has the full disk.Confirm Smsts.log imaging status is the full disk.
Three bounded controls remained accepted, including Confirm all files were downloaded, as recorded in PolicyAgent.log. The correction needs to reject auxiliary-led unbound broad targets at all four boundaries without removing that bounded observation. Add a permanent four-boundary regression matrix for the no-trailing-participle forms.
CodeRabbit independently reported the same exact-head P1 in a substantive review: #353 (comment). The older formal CodeRabbit approval is stale and does not apply to this SHA.
Verification at the exact head:
- focused finding contract: 72 passed
- complete spine contract: 135 passed
- full parser crate: passed
- strict parser Clippy: passed
wasm32-unknown-unknowncheck: passed- TypeScript
tsc --noEmit: passed with the existing dependency tree exposed to the detached worktree - scoped
rustfmt --checkfor all five changed Rust files: passed - repository
cargo fmt --check --all: same 17-file / 24-hunk unrelated baseline at base and head git diff --check: passed
The remaining reviewed controls are intact: coverage-only findings are restricted to InsufficientEvidence; invalid and unregistered profiled keys fail closed; message, path-adjacent, structured, component, and ccmSourceFile redaction tests pass with safe controls; mutable finding serialization normalizes and validates; recursive deserialization remains strict; no dependency, ParserKind::Sccm, public LogEntry, native I/O, Windows/Tauri, network, process, or wasm-specific implementation delta was introduced.
Keep PR #353 draft. This review does not authorize ready-for-review, merge, or issue closure.
#318 review correction — exact head
|
|
@coderabbitai review Please perform a fresh substantive review of exact head Focus on whether auxiliary certainty claims can still smuggle an unbounded file, disk, drive, directory, or filesystem request across builder, direct validation, serialization, or deserialization, including word-order and punctuation variants. Also verify that bounded exact-artifact state observations remain accepted. This request supersedes reviews of earlier heads. A status-only, skipped, rate-limited, or stale response will not be treated as approval. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
base="25b37333affde22b4ef8a19f4a5f3d89c082b599"
head="889eaa1da73b94b6d9cc7de76465fe8997ff9626"
prior="5c0fda213686fd9ddb12a29723c601f675836567"
printf 'checked-out: '; git rev-parse HEAD
printf 'base object: '; git cat-file -t "$base"
printf 'head object: '; git cat-file -t "$head"
echo "== Exact changed paths and whitespace =="
git diff --name-status "$base" "$head"
git diff --stat "$base" "$head"
git diff --check "$base" "$head"
echo "== Latest correction delta =="
git diff --no-ext-diff "$prior" "$head" -- \
crates/cmtraceopen-parser/src/sccm/findings.rs \
crates/cmtraceopen-parser/tests/sccm_spine_contract.rs
echo "== Exact-head SCCM contract outline =="
git show "$head:crates/cmtraceopen-parser/src/sccm/findings.rs" > /tmp/findings-889eaa1.rs
ast-grep outline /tmp/findings-889eaa1.rs --items all --type struct,enum,impl,function \
--match 'Sccm|Finding|Artifact|Confirm|Evidence|Coverage|Correlation|Confidence|Phase|Terminal' || true
echo "== Validation and public serde paths =="
git show "$head:crates/cmtraceopen-parser/src/sccm/findings.rs" | cat -n | \
sed -n '150,760p; 1940,2300p'
echo "== Confirm grammar, target checks, clause splitting, and scope vocabulary =="
git show "$head:crates/cmtraceopen-parser/src/sccm/findings.rs" | cat -n | \
sed -n '700,1945p'
echo "== Passive/broad confirmation matrices and bounded controls =="
git show "$head:crates/cmtraceopen-parser/tests/sccm_spine_contract.rs" | \
rg -n -A180 -B30 \
'passive.*unbounded|bounded.*auxiliary|all files|full disk|drive|directory|filesystem|had|has|have|are|be|been|being|is|was|were|Confirm|serialize|deserializ|direct.validation' || true
echo "== Exact call paths and parser/public-surface scope =="
git grep -n -E \
'validate_artifact_request|reason_scope_is_within_catalog_artifact|confirmation_clause_is_non_authorizing|has_passive_unbounded_confirmation_request|SccmFindingBuilder::new|impl (Serialize|Deserialize) for SccmFinding|from_(str|slice|value).*SccmFinding|to_(string|value).*SccmFinding' \
"$head" -- crates/cmtraceopen-parser || true
git diff --no-ext-diff "$base" "$head" -- Cargo.toml Cargo.lock
git diff --no-ext-diff "$base" "$head" -- crates/cmtraceopen-parser/src | \
rg -n -C3 '^\+.*(LogEntry|ParserKind|std::fs|std::net|std::process|tauri|windows|winapi|tokio|reqwest|unsafe)|^\-.*(LogEntry|ParserKind)' || trueLength of output: 118569 🏁 Script executed: #!/bin/bash
set -euo pipefail
head="889eaa1da73b94b6d9cc7de76465fe8997ff9626"
echo "== Dispatch and confirmation helpers =="
git show "$head:crates/cmtraceopen-parser/src/sccm/findings.rs" | cat -n | \
sed -n '810,900p; 1160,1365p'
echo "== Broad-target and confirmation vocabulary helpers =="
git show "$head:crates/cmtraceopen-parser/src/sccm/findings.rs" | cat -n | \
sed -n '1365,1545p; 1690,1945p'
echo "== Tokenization and identity-range behavior =="
git show "$head:crates/cmtraceopen-parser/src/sccm/findings.rs" | cat -n | \
sed -n '1545,1695p; 2020,2145p'
echo "== Four-boundary test helper and passive/bounded matrices =="
git show "$head:crates/cmtraceopen-parser/tests/sccm_spine_contract.rs" | cat -n | \
sed -n '450,590p; 2340,2455p'
echo "== Exact latest test delta =="
git diff --no-ext-diff 5c0fda213686fd9ddb12a29723c601f675836567 "$head" -- \
crates/cmtraceopen-parser/src/sccm/findings.rs \
crates/cmtraceopen-parser/tests/sccm_spine_contract.rs
echo "== Broad-target terminology and all Confirm regression literals =="
git show "$head:crates/cmtraceopen-parser/src/sccm/findings.rs" | \
rg -n -C5 'broad|all.*files|full.*disk|drive|directory|filesystem|auxiliar|certainty|passive'
git show "$head:crates/cmtraceopen-parser/tests/sccm_spine_contract.rs" | \
rg -n -C5 'Confirm .*?(all files|full disk|drive|directory|filesystem)|bounded auxiliary|passive unbounded'Length of output: 74482
Finding
I also confirmed the following:
Keep PR You are interacting with an AI system. |
Exact-head CodeRabbit P1 corrected — head
|
|
@coderabbitai review Please perform a fresh substantive review of exact head Re-test broad file and disk targets both before and after every auxiliary, including punctuation variants, at builder, direct validation, validating serialization, and strict deserialization boundaries. Verify that only explicit bounded exact-artifact state observations remain accepted. This request supersedes all earlier-head reviews. A skipped, status-only, stale, or rate-limited response is not approval. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
base="25b37333affde22b4ef8a19f4a5f3d89c082b599"
head="29d3bc54ae9eb68aa8cdef6588adaccaadb8de7d"
prior="889eaa1da73b94b6d9cc7de76465fe8997ff9626"
echo "== Exact revisions =="
printf 'checked-out: '; git rev-parse HEAD
printf 'base: '; git cat-file -t "$base"
printf 'head: '; git cat-file -t "$head"
echo "== Range scope and whitespace =="
git diff --name-status "$base" "$head"
git diff --stat "$base" "$head"
git diff --check "$base" "$head"
echo "== Latest correction delta =="
git diff --no-ext-diff "$prior" "$head" -- \
crates/cmtraceopen-parser/src/sccm/findings.rs \
crates/cmtraceopen-parser/tests/sccm_spine_contract.rs
echo "== Findings declaration map =="
git show "$head:crates/cmtraceopen-parser/src/sccm/findings.rs" > /tmp/findings-29d3bc54.rs
ast-grep outline /tmp/findings-29d3bc54.rs --items all --type struct,enum,impl,function \
--match 'Sccm|Finding|Artifact|Confirm|Evidence|Coverage|Correlation|Confidence|Phase|Terminal' || true
echo "== Public validation and serde paths =="
git show "$head:crates/cmtraceopen-parser/src/sccm/findings.rs" | cat -n | \
sed -n '150,760p; 1940,2325p'
echo "== Artifact validation, Confirm grammar, and broad target checks =="
git show "$head:crates/cmtraceopen-parser/src/sccm/findings.rs" | cat -n | \
sed -n '700,1960p'
echo "== Tokenization and all confirmation helper definitions/callers =="
git show "$head:crates/cmtraceopen-parser/src/sccm/findings.rs" | \
rg -n -C6 \
'confirmation_clause_is_non_authorizing|has_passive_unbounded_confirmation_request|is_.*auxiliary|is_broad|all.*files|full.*disk|drive|directory|filesystem|request_clauses|tokenize_request_reason|is_collection_action'
echo "== Passive broad-target and bounded-control matrices =="
git show "$head:crates/cmtraceopen-parser/tests/sccm_spine_contract.rs" | cat -n | \
rg -n -A210 -B35 \
'passive.*unbounded|inverse.order|bounded.*auxiliary|bounded.*named|all files|full disk|drive|directory|filesystem|had|has|have|are|be|been|being|is|was|were|Confirm|serialize|deserializ|direct.validation' || true
echo "== Four-boundary helper implementation and exact test bodies =="
git show "$head:crates/cmtraceopen-parser/tests/sccm_spine_contract.rs" | cat -n | \
sed -n '440,610p; 2320,2515p'
echo "== Parser/platform/public surface delta =="
git diff --no-ext-diff "$base" "$head" -- Cargo.toml Cargo.lock
git diff --no-ext-diff "$base" "$head" -- crates/cmtraceopen-parser/src | \
rg -n -C3 '^\+.*(LogEntry|ParserKind|std::fs|std::net|std::process|tauri|windows|winapi|tokio|reqwest|unsafe)|^\-.*(LogEntry|ParserKind)' || true
git diff --name-only "$base" "$head" | sortLength of output: 117887 🏁 Script executed: #!/bin/bash
set -euo pipefail
head="29d3bc54ae9eb68aa8cdef6588adaccaadb8de7d"
prior="889eaa1da73b94b6d9cc7de76465fe8997ff9626"
echo "== Latest correction only =="
git diff --no-ext-diff "$prior" "$head" -- \
crates/cmtraceopen-parser/src/sccm/findings.rs \
crates/cmtraceopen-parser/tests/sccm_spine_contract.rs
echo "== Exact confirmation helper implementation =="
git show "$head:crates/cmtraceopen-parser/src/sccm/findings.rs" | nl -ba | \
sed -n '1160,1435p'
echo "== Exact broad-target and auxiliary vocabularies =="
git show "$head:crates/cmtraceopen-parser/src/sccm/findings.rs" | nl -ba | \
sed -n '1435,1595p; 1690,1815p'
echo "== Exact request validation dispatch and serde validation sequencing =="
git show "$head:crates/cmtraceopen-parser/src/sccm/findings.rs" | nl -ba | \
sed -n '190,460p; 780,915p'
echo "== Complete passive-reject and bounded-control constants =="
git show "$head:crates/cmtraceopen-parser/tests/sccm_spine_contract.rs" | nl -ba | \
sed -n '320,465p'
echo "== Exact four-boundary regression test bodies =="
git show "$head:crates/cmtraceopen-parser/tests/sccm_spine_contract.rs" | nl -ba | \
sed -n '2280,2465p'
echo "== Punctuation handling and complete test reason corpus =="
git show "$head:crates/cmtraceopen-parser/src/sccm/findings.rs" | \
rg -n -A45 -B10 'fn request_clauses|fn tokenize_request_reason|strong.punctuation|split'
git show "$head:crates/cmtraceopen-parser/tests/sccm_spine_contract.rs" | \
rg -n -C3 'Confirm .*?(all files|full disk|drive|directory|filesystem)|all files.*Confirm|full disk.*Confirm|,.*(has|have|had|are|be|been|being|is|was|were)' || trueLength of output: 5047 🏁 Script executed: #!/bin/bash
set -euo pipefail
head="29d3bc54ae9eb68aa8cdef6588adaccaadb8de7d"
echo "== Confirmation helper implementation =="
git show "$head:crates/cmtraceopen-parser/src/sccm/findings.rs" | cat -n | \
sed -n '1160,1435p'
echo "== Broad-target, auxiliary, and identity helper definitions =="
git show "$head:crates/cmtraceopen-parser/src/sccm/findings.rs" | cat -n | \
sed -n '1435,1595p; 1690,1815p'
echo "== Artifact request dispatch and finding serde validation paths =="
git show "$head:crates/cmtraceopen-parser/src/sccm/findings.rs" | cat -n | \
sed -n '190,460p; 780,915p'
echo "== Passive-reject and bounded-control constants =="
git show "$head:crates/cmtraceopen-parser/tests/sccm_spine_contract.rs" | cat -n | \
sed -n '320,465p'
echo "== Four-boundary test bodies =="
git show "$head:crates/cmtraceopen-parser/tests/sccm_spine_contract.rs" | cat -n | \
sed -n '2280,2465p'
echo "== Clause splitting and tokenization =="
git show "$head:crates/cmtraceopen-parser/src/sccm/findings.rs" | \
rg -n -A55 -B12 'fn request_clauses|fn tokenize_request_reason|fn reason_scope_is_within_catalog_artifact'
echo "== All confirmation broad-target literals =="
git show "$head:crates/cmtraceopen-parser/tests/sccm_spine_contract.rs" | \
rg -n -C2 'Confirm .*?(all files|every file|full disk|whole disk|drive|directory|filesystem)|all files.*Confirm|full disk.*Confirm' || trueLength of output: 67931
No blocking findings. Confirmed in this pass:
PR You are interacting with an AI system. |
adamgell
left a comment
adamgell
left a comment
There was a problem hiding this comment.
Independent exact-head API/security review — PASS
Reviewed exact range 25b37333affde22b4ef8a19f4a5f3d89c082b599..29d3bc54ae9eb68aa8cdef6588adaccaadb8de7d in a new clean detached worktree. The remote PR head and base were re-read immediately before submission and match those SHAs. No blocking finding remains.
Certainty-claim closure
- The committed 36-case passive/inverse broad-target matrix rejects all 144 builder, direct
SccmFinding::validate(), validating serializer, and strict deserializer boundaries. - The committed 10-case bounded auxiliary matrix accepts all 40 corresponding boundaries.
- A disposable independent matrix added 16 new unsafe file/disk variants across auxiliary-before-broad and broad-before-auxiliary order, with colon, comma, parentheses, and em-dash forms. All 64 public-boundary attempts were rejected.
- The same probe added eight bounded controls for negative state observations and exact-artifact download state. All 32 public-boundary attempts were accepted. This includes identity-before/after-auxiliary order and DataTransferService/PolicyAgent exact artifact forms.
- The disposable probe was removed; the exact-head review worktree is clean.
Earlier blocker audit
- Privacy/redaction tests cover message, component,
ccmSourceFile, path-adjacent identities, structured account fields, UPN/local identities, credentials/tokens, deterministic projection, and raw context omission. The focused evidence filter passed 30 tests. A local CodeRabbit minor noted the trailing\bon Windows identity patterns; a disposable$/-suffix probe confirmed the account names remain redacted on every public string surface. Its proposed negative lookahead is unsupported by Rust regex and no raw identity leak was reproduced, so this is not a blocker. - Coverage-only
AccessDeniedremains valid only forInsufficientEvidence; Symptom, LikelyContributor, ConfirmedFailure, and BlockedOrDeferred were rejected at builder, validate, serialize, and deserialize boundaries. - High ConfirmedFailure still requires cited terminal evidence because the parser-owned stable profile registry is empty. Forged, unknown, unregistered, uncited, duplicate, low, same-time, and keyless paths fail closed. High LikelyContributor remains capped without terminal evidence.
- Recursive wire strictness, canonical role/phase handling, evidence identity/range consistency, deterministic sort/dedup, bounded catalog requests, and validating serialization/deserialization remain intact.
- The exact range changes only five SCCM parser/test files. There is no dependency change,
ParserKind::Sccm, publicLogEntrychange, native/Windows/Tauri/network/database/process I/O, or live acceptance claim.
Fresh verification
- finding target: 73 passed
- committed spine: 136 passed
- full parser crate: 729 passed
- repository-default strict Clippy: passed
- Rust 1.88 wasm32 check: passed
- TypeScript 7.0.2
tsc --noEmit: passed - Rust 1.88 owned-file
rustfmt --check: passed - worktree and exact base/head
git diff --check: passed - full-workspace Rust 1.88 formatting: unchanged unrelated baseline of 17 files / 24 hunks at base and head, none in the five changed files
- Rust 1.88 strict Clippy: unchanged unrelated baseline of 59
uninlined_format_argslints at base and head, none in the five changed files; default strict Clippy is green
Fresh hosted CodeRabbit performed a substantive exact-head review with no blocker: #353 (comment). Authenticated local CodeRabbit 0.7.1 completed against the same exact range with the one non-blocking suffix note evaluated above.
Both previously owner-held review threads were independently reproduced through their permanent four-boundary tests and are now resolved; the PR has zero unresolved threads.
This is an exact-head PASS for the #318 findings slice. Keep PR #353 draft and do not merge, mark ready, or close #318 solely from this review; the coordinator still owns the foundation merge gate and downstream restack order.
There was a problem hiding this comment.
Pull request overview
This PR implements SCCM “evidence-backed findings” contracts at the parser-crate layer by introducing a new sccm::findings module that defines public finding/phase/confidence/terminal-evidence/coverage-gap/artifact-request types, plus strict validation + deterministic normalization across build/validate/serialize/deserialize boundaries.
Changes:
- Add a new SCCM finding contract surface (
SccmFinding*types +SccmFindingBuilder) with strict validation and canonical (sorted/deduped) serialization/deserialization. - Tighten SCCM public redaction to cover additional identity shapes (including path-adjacent identities) and extend sensitive-label detection.
- Add extensive contract/regression tests enforcing the new constraints across builder, direct
validate(), serialization, and deserialization.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| crates/cmtraceopen-parser/tests/sccm_spine_contract.rs | Adds broad regression coverage for new SCCM finding validation, canonicalization, artifact-request bounds, and expanded identity-redaction guarantees. |
| crates/cmtraceopen-parser/src/sccm/models.rs | Hardens SccmRole serde by requiring canonical unknown-role values that cannot shadow declared roles. |
| crates/cmtraceopen-parser/src/sccm/mod.rs | Registers and re-exports the new findings module from sccm. |
| crates/cmtraceopen-parser/src/sccm/findings.rs | Introduces the core SCCM finding contracts (types + wire format + normalization + validation + builder). |
| crates/cmtraceopen-parser/src/sccm/evidence.rs | Expands public redaction to catch additional identity patterns (including user-path-adjacent identities) and redacts identity-bearing component/source-file surfaces. |
Scope
Implements #318 Task 7 from the SCCM diagnostic-spine plan as a pure parser-crate finding builder and validator, on exact base
25b37333affde22b4ef8a19f4a5f3d89c082b599.Confirmclauses to match a finite positive confirmation vocabulary, include a confirmation subject or exact catalog identity, reject unbound dotted targets, and retain the existing bounded state-observation exceptions.MP_*basenames, accepts every declared catalog basename/logical alias, and rejects prefix/suffix/lookalike identities.artifactIdas one identity: conflicting role/state is rejected; exact duplicates deterministically deduplicate.No native I/O, Tauri, Windows dependency, collection, reducer, correlation engine,
ParserKind, CCM grammar, publicLogEntry, workflow fixture, dependency, or live Windows acceptance change is included.Test-first evidence
Permanent four-boundary regressions cover builder, direct
validate(), validating serialization, and deserialization:MP_*basenames: 20 false rejections RED → GREEN.Confirmimperative/gerund/nominal/dotted-target variants from the latest CodeRabbit P1: 40 accepted boundaries RED → GREEN.must have all files provided/must have the full disk provided): 8 accepted boundaries RED → GREEN; the two literal CodeRabbitmust include/must provideexamples remain permanent already-green rejection controls.DOMAIN\\Userpayloads leaked through both the public evidence message and JSON RED → GREEN; deterministic redaction now preserves only the safe surrounding path fragments.Verification on exact head
a515f33858be599759dd84e02353bc1f92f4a16fcargo +1.88.0 test --locked -p cmtraceopen-parser --test sccm_spine_contract finding_ -- --nocapture— 71 passed, 0 failedcargo +1.88.0 test --locked -p cmtraceopen-parser— 726 passed, 0 failedcargo clippy -p cmtraceopen-parser --all-targets -- -D warnings— passedcargo +1.88.0 check --locked -p cmtraceopen-parser --target wasm32-unknown-unknown— passednpx tsc --noEmit— passedrustfmt --checkon the three changed files — passedgit diff --check,git diff --check ca7ee943794f0654c2502606088743ac0b9db7c0..a515f33858be599759dd84e02353bc1f92f4a16f, andgit diff --check 25b37333affde22b4ef8a19f4a5f3d89c082b599..a515f33858be599759dd84e02353bc1f92f4a16f— passedKnown unrelated baseline: repository-wide
cargo fmt --all -- --checkremains red only in the same 17 unrelated files, which this issue-owned slice does not modify. The repository-prescribed strict current-toolchain Clippy command above passes.Review gate
Independent review of
d6d5c697a4fa756078366bebe34b14519f5a0c5ereproduced three P1 shared-contract failures: passive broad confirmation wording, path-adjacent Windows identity leakage, and unvalidated/malformed Strong/Exact finding keys: #353 (review).Commit
ca7ee943794f0654c2502606088743ac0b9db7c0closes all three with permanent public-boundary regressions while preserving bounded confirmation, safe path fragments, Low-confidence unprofiled/unknown keys, parser purity, and wasm32 compatibility.CodeRabbit's substantive review of that exact head accepted the key/redaction corrections and found a remaining confirmation-order P1: #353 (comment). Its two literal examples were already rejected; a dynamic equivalent using allowed tokens reproduced the actual root at all four boundaries. Commit
a515f33858be599759dd84e02353bc1f92f4a16fnow searches the whole confirmation body for an unbound broad target and closes the reproduced 8/8 trailing-object accepts without expanding the finite action vocabulary.Hosted CodeRabbit completed a substantive review of exact range
25b37333affde22b4ef8a19f4a5f3d89c082b599..a515f33858be599759dd84e02353bc1f92f4a16fwith no blocking findings: #353 (comment). A different independent exact-head API/security review is still required before this PR may leave draft. A green status or approval attached to an older SHA is not sufficient. Historical blocker comments remain unresolved as review history.Do not mark ready, merge, close #318, authorize downstream production reducers/correlation, or claim live Windows acceptance from this PR.
Refs #318