feat(intune): parse Company Portal Windows LocalState logs - #460
Merged
Conversation
Adds `cmtraceopen_parser::intune::portal::windows::company_portal::logs`, a dedicated parser plus canonical evidence document for `%LOCALAPPDATA%\Packages\Microsoft.CompanyPortal_8wekyb3d8bbwe\LocalState\Log_<n>.log` and the sibling `Log.<BridgeName>_<n>.log` bridge logs. Evidence basis and its limitation --------------------------------- Microsoft documents the path and the `Log_<n>.log` pattern but not the record grammar. Exactly ONE verbatim record has ever been published, from Company Portal app version 12-0-0: 2024-11-15T16:50:07.2850341Z INFO Event None 0 <guid> 12-0-0 [Configuration Manager Trace Listener] ... Everything here is derived from that single record, so the grammar is version-scoped from the start: - records are read with GrammarVersion::V1; - 12-0-0 is the only validated app version. Any other version still parses with V1 (it is the only grammar that exists) but downgrades the selection to ParserProvenance::Heuristic and the document to Experimental / Low confidence, and names the gap in coverage; - document confidence never reaches High. Raising it requires a second app version captured from a real device. Encoding, newline style, rotation ordering, the full severity vocabulary, and whether payloads genuinely span lines are all unproven from public evidence. Each is handled defensively rather than assumed, and the open items are recorded in the module docs. Detection safety ---------------- `Log_<n>.log` is a generic name that any UWP package can use, so the file name only nominates a candidate. Confirmation requires field 6 to be a hyphenated GUID and field 7 to be a dash-separated version triple. Two negative fixtures prove it: a column-aligned unrelated UWP log with ISO instants and a severity column is refused even when it sits at the exact Company Portal path, and a generic timestamped log stays on the generic parser. Losslessness and privacy ------------------------ The nested legacy ConfigMgr trace text inside the message — including its day-first date — is never stripped or reinterpreted. Records that fail validation keep their original text and are reported through parse_errors and a coverage row rather than dropped. Dedicated severity wins over keyword inference; only an unrecognized token defers to it. The evidence document is redacted by default and reuses the existing ESP free-text rule table rather than growing a second one; the unredacted form is an explicitly named local-only opt-out. The viewer's LogEntry path is never redacted, because it has to show the file the user opened. Also: the CI parser-crate step now runs every test target in the crate (it named a single target, so new targets ran nowhere), adds a parser-crate clippy gate, and `.gitignore`'s `Logs/` rule is un-ignored for the new `logs/` directories, which it was matching case-insensitively on macOS and Windows checkouts. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A code review of this branch produced six findings. None reached the
confidence bar to post as blocking review comments, but four were verified
accurate and are documentation/API-surface defects worth correcting rather
than shipping.
Before: several doc comments claimed more than the implementation delivers.
* models.rs described field 5 as a "monotonic sequence value" while
grammar.rs documented the same field as "semantics unproven". One published
record cannot establish monotonicity, and nothing checks it. The claim is
removed; the field is now described as an unsigned integer of unproven
semantics, which is what the evidence supports.
* CompanyPortalTimestampKind::Invalid was documented as "the field had the
right shape but is not a real instant", but nothing ever constructed it:
parse_utc_instant returns None for that input, so the record is framed
Malformed and reaches the document with timestamp: None. The variant was
dead public API on a published crate describing behavior that does not
happen. Removed, and the enum now documents what actually occurs. Dropping
a half-resolved timestamp is the correct behavior, so only the type and its
doc change.
* matches_company_portal_log_record claimed it was "used by parser::detect".
parser::detect calls classify_line directly, because it needs the
classification to count validated app versions rather than a bool. The
function is the house-convention boolean wrapper; the doc now says so.
* The module claimed losslessness in three places while framing.rs strips
trailing whitespace from every line and drops blank lines entirely. Neither
is reversible from raw_text. The claims are narrowed to what holds — a
record the grammar cannot read is still reported rather than dropped — and
framing.rs now names both exceptions explicitly, including which rule has
to change if a multi-line payload containing a blank line is ever observed.
Why this seam: these are contract statements on a crate published to
crates.io, in a module whose entire premise is not claiming more than the
evidence proves. A doc that overclaims is the same defect class the module
exists to avoid.
Verified on this commit:
cargo test --locked -p cmtraceopen-parser
-> lib 403 passed, company_portal_windows_logs 32 passed,
esp_diagnostics 222 passed, 0 failed
cargo clippy --locked -p cmtraceopen-parser --all-targets -- -D warnings
-> clean
cargo fmt --check --all
-> no diff in any file this branch touches
Refs #366
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a version-aware Company Portal Windows log parser with structured records, redaction, detection, encoding support, desktop integration, frontend tail amendments, fixture protection, documentation, and CI validation. ChangesCompany Portal Windows parser
Estimated code review effort: 5 (Critical) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant FileLoader
participant CompanyPortalParser
participant TailSession
participant FrontendWatcher
participant LogStore
FileLoader->>CompanyPortalParser: parse initial Company Portal records
CompanyPortalParser->>FileLoader: return entries and initial logical record
FileLoader->>TailSession: start with initial logical record
TailSession->>FrontendWatcher: emit entries, amendments, and line coverage
FrontendWatcher->>LogStore: validate payload and update state
Possibly related issues
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
added
enhancement
Owner
Author
|
@coderabbitai full review — exact-head review requested for Please assess this as the bounded raw-format Company Portal parser described in the PR: content-confirmed detection, version-scoped provenance, default evidence redaction, synthetic fixtures, and parser/UI registration. It does not claim live Windows collection, a second real-version capture, or semantic root-cause analysis. An independent review corrected native parser-format inventory coverage and untrusted severity redaction before this replacement PR was published. A fresh local CodeRabbit pass surfaced an untriaged public re-export-surface suggestion; please inspect the exact public API rather than treating that automated suggestion as accepted. |
This was referenced Aug 3, 2026
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '== HEAD =='
git rev-parse HEAD
printf '%s\n' '== Diff stat against main =='
git diff --stat origin/main...HEAD
printf '%s\n' '== Changed paths =='
git diff --name-status origin/main...HEAD
printf '%s\n' '== Public Rust exports and Company Portal references =='
rg -n -C 3 --glob '*.rs' \
'pub\s+(use|mod|struct|enum|fn)|Company\s*Portal|company_portal|company-portal' .
printf '%s\n' '== TypeScript registration references =='
rg -n -C 3 --glob '*.{ts,tsx}' \
'Company\s*Portal|company_portal|company-portal' .
printf '%s\n' '== Source outlines for changed Rust files =='
git diff --name-only --diff-filter=AM origin/main...HEAD \
| grep -E '\.rs$' \
| while IFS= read -r file; do
printf '\n-- %s --\n' "$file"
ast-grep outline "$file" --items all || true
doneLength of output: 175184 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '== Exact head and changed paths =='
git rev-parse HEAD
git diff --name-status origin/main...HEAD | grep -Ei 'company|parser/(detect|mod)\.rs|models/log_entry\.rs|bundle_ops\.rs|parser_supported_formats'
printf '%s\n' '== Public module chain =='
for file in \
crates/cmtraceopen-parser/src/lib.rs \
crates/cmtraceopen-parser/src/intune/mod.rs \
crates/cmtraceopen-parser/src/intune/portal/mod.rs \
crates/cmtraceopen-parser/src/intune/portal/windows/mod.rs \
crates/cmtraceopen-parser/src/intune/portal/windows/company_portal/mod.rs \
crates/cmtraceopen-parser/src/intune/portal/windows/company_portal/logs/mod.rs
do
if [ -f "$file" ]; then
printf '\n-- %s --\n' "$file"
cat -n "$file"
fi
done
printf '%s\n' '== Changed Company Portal diff =='
git diff --find-renames --find-copies --unified=40 origin/main...HEAD -- \
crates/cmtraceopen-parser/src/intune/portal/windows/company_portal \
crates/cmtraceopen-parser/src/parser/detect.rs \
crates/cmtraceopen-parser/src/parser/mod.rs \
crates/cmtraceopen-parser/src/models/log_entry.rs \
src-tauri/src/commands/bundle_ops.rs \
src-tauri/tests/parser_supported_formats.rs \
crates/cmtraceopen-parser/tests/company_portal_windows_logs.rs
printf '%s\n' '== Parser selection and dispatch =='
sed -n '430,570p' crates/cmtraceopen-parser/src/parser/detect.rs
sed -n '58,215p' crates/cmtraceopen-parser/src/parser/mod.rs
printf '%s\n' '== Detector, document projection, and entry projection =='
cat -n crates/cmtraceopen-parser/src/intune/portal/windows/company_portal/logs/detect.rs
cat -n crates/cmtraceopen-parser/src/intune/portal/windows/company_portal/logs/document.rs
cat -n crates/cmtraceopen-parser/src/intune/portal/windows/company_portal/logs/entries.rsLength of output: 209848 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '== Logical-record consumers and tail implementation =='
rg -n -C 8 --glob '*.rs' \
'parse_one_line\s*\(|parse_lines_with_selection\s*\(|RecordFraming::LogicalRecord|record_framing' \
src-tauri/src crates/cmtraceopen-parser/src
printf '%s\n' '== Tail reader source =='
sed -n '100,330p' src-tauri/src/watcher/tail.rs
sed -n '330,525p' src-tauri/src/watcher/tail.rs
printf '%s\n' '== Parser enum match sites outside changed files =='
rg -n -C 5 --glob '*.{rs,ts,tsx}' \
'ParserKind::|ParserImplementation::|companyPortal' \
src-tauri/src src crates/cmtraceopen-parser/src \
| grep -E 'ParserKind::|ParserImplementation::|companyPortal|match parser|match .*implementation' || true
printf '%s\n' '== UI format labels and parser selection serialization =='
sed -n '250,350p' src/stores/log-store.ts
rg -n -C 6 --glob '*.{rs,ts,tsx}' \
'parserSelection|parser_selection|formatDetected|format_detected' \
src src-tauri/src crates/cmtraceopen-parser/srcLength of output: 214166
Defect: The tail reader does not preserve Company Portal logical records across updates. In Add Company Portal framing to the stateful logical-record path, or retain its final parsed header until the next record header, rotation, or debounce flush. Add a tail regression test that appends a header and continuation in separate writes. The public re-export suggestion is not valid. The API is already public at:
The public module chain and the ✅ Action performedFull review finished. |
Owner
Author
Exact-head review checkpoint — not merge-readyIndependent and hosted review have identified two verified no-go behaviors on
The recovery branch is intentionally unchanged. A new isolated correction branch begins at this exact head with separate failing tests and issue-scoped commits; it will require a fresh independent review before any push. Local CodeRabbit review reported nine findings. Disposition after code inspection:
Hosted CI is still running. The Windows ESP diagnostic job currently reports failure while its enclosing run is still finalizing; its log is not available yet, so no cause is claimed. |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds first dedicated raw-format parser for Windows Company Portal LocalState logs (Log_<n>.log and Log.<BridgeName>_<n>.log) to the cmtraceopen-parser Intune/Portal surface, including conservative detection, logical-record framing, LogEntry projection, redacted evidence document output, and end-to-end registration in the app + frontend.
Changes:
- Implement
cmtraceopen_parser::intune::portal::windows::company_portal::logs(grammar, detection, framing, viewer entries, evidence document, redaction, models). - Register the new parser kind/implementation across backend + frontend, with corpus fixtures and contract tests.
- Update references/docs and CI/gitattributes to support byte-sensitive fixtures and run the full parser-crate test/clippy gates.
Reviewed changes
Copilot reviewed 22 out of 41 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/types/log.ts | Add companyPortal to frontend parser kind/implementation unions. |
| src/stores/log-store.ts | Add UI labels for companyPortal parser kind/implementation. |
| src/lib/column-config.ts | Define default column set for companyPortal logs. |
| src-tauri/tests/parser_supported_formats.rs | Add Company Portal to supported-parser contract + add fixture-based detection/parse tests. |
| src-tauri/tests/corpus/company_portal/negative/Log_1.log | Add negative corpus sample to ensure detection doesn’t rely on filename/path alone. |
| src-tauri/tests/corpus/company_portal/clean/Log_1.log | Add positive corpus sample used by native app contract tests. |
| src-tauri/src/commands/bundle_ops.rs | Add selection description string for Company Portal parser. |
| references/log-intune-reference.md | Document Company Portal log grammar and version-scoping limitations. |
| crates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v13-4-2/unknown-app-version/Log_1.log | Fixture exercising unknown-version downgrade behavior. |
| crates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/truncated-boundaries/Log_1.log | Fixture for truncated first/last record handling. |
| crates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/severity-levels/Log_1.log | Fixture covering multiple severity tokens (incl. unknown). |
| crates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/same-timestamp-distinct-activity/Log_1.log | Fixture ensuring distinct activity IDs aren’t merged when timestamps match. |
| crates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/rotation/Log_2.log | Fixture for rotated member behavior. |
| crates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/rotation/Log_1.log | Fixture for rotated member behavior (current). |
| crates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/redaction/Log_1.log | Fixture for redaction of synthetic sensitive values. |
| crates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/negative-unrelated-uwp/Log_1.log | Negative fixture for unrelated UWP log collision. |
| crates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/negative-generic-timestamped/Log_1.log | Negative fixture for generic timestamped text logs. |
| crates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/multiline-continuation/Log_1.log | Fixture for logical-record framing of continuation lines. |
| crates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/malformed-structural-token/Log_1.log | Fixture for malformed structural token handling. |
| crates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/invalid-timestamp/Log_1.log | Fixture ensuring invalid timestamps become parse errors, not “best-effort” timestamps. |
| crates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/encoding-utf8-nobom/Log_1.log | UTF-8 no-BOM encoding fixture. |
| crates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/encoding-utf8-bom/Log_1.log | UTF-8 BOM encoding fixture. |
| crates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/code-tokens/Log_1.log | Fixture ensuring known/unknown code tokens are preserved. |
| crates/cmtraceopen-parser/tests/company_portal_windows_logs.rs | Add dedicated Company Portal contract test suite (fixtures, framing, redaction, downgrade, negatives). |
| crates/cmtraceopen-parser/src/parser/mod.rs | Route ParserImplementation::CompanyPortal to new parser entrypoint. |
| crates/cmtraceopen-parser/src/parser/detect.rs | Add Company Portal detection (path hint + strict record-structure confirmation + heuristic downgrade). |
| crates/cmtraceopen-parser/src/models/log_entry.rs | Add CompanyPortal variants to ParserKind and ParserImplementation. |
| crates/cmtraceopen-parser/src/intune/portal/windows/company_portal/logs/redaction.rs | Implement redacted export projection using shared ESP redaction rules. |
| crates/cmtraceopen-parser/src/intune/portal/windows/company_portal/logs/models.rs | Define serde-stable evidence document wire types (records, coverage, schema versioning). |
| crates/cmtraceopen-parser/src/intune/portal/windows/company_portal/logs/mod.rs | Replace skeleton with full module docs/exports and module wiring. |
| crates/cmtraceopen-parser/src/intune/portal/windows/company_portal/logs/grammar.rs | Implement V1 record grammar parsing and strict structure checks. |
| crates/cmtraceopen-parser/src/intune/portal/windows/company_portal/logs/framing.rs | Implement logical-record framing shared by viewer and evidence document. |
| crates/cmtraceopen-parser/src/intune/portal/windows/company_portal/logs/entries.rs | Implement LogEntry projection for viewer (no redaction). |
| crates/cmtraceopen-parser/src/intune/portal/windows/company_portal/logs/document.rs | Build canonical evidence document (redacted-by-default) + coverage reporting. |
| crates/cmtraceopen-parser/src/intune/portal/windows/company_portal/logs/detect.rs | Implement file-identity parsing and per-line structural classification for detection. |
| crates/cmtraceopen-parser/src/esp/redaction.rs | Expose redaction entrypoint as pub(crate) for reuse by other evidence modules. |
| crates/cmtraceopen-parser/src/esp/mod.rs | Re-export crate-internal redact_text helper for sibling modules. |
| Cargo.lock | Update lockfile for workspace version bump to 1.5.1. |
| .github/workflows/cmtrace-ci.yml | Run full parser-crate tests + parser-crate clippy in CI (not just ESP suite). |
| .gitattributes | Mark parser fixtures/corpus as byte-sensitive (no EOL normalization; disable whitespace errors). |
Comment on lines
+58
to
+61
| /// `true` when the record could not be read as a well-formed record. | ||
| pub(super) fn is_parse_error(&self) -> bool { | ||
| !matches!(self.kind, FramedRecordKind::Record(_)) | ||
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 @.github/workflows/cmtrace-ci.yml:
- Around line 245-247: Update the comment near the Windows parser-crate targets
to remove the claim that this is the only place parser tests execute, and state
instead that the job adds Windows-specific validation. Leave the workflow
commands unchanged.
In
`@crates/cmtraceopen-parser/src/intune/portal/windows/company_portal/logs/detect.rs`:
- Around line 17-39: Remove the constant is_record field from
CompanyPortalLineClassification and delete its documentation and initializer in
classify_line. Keep classify_line returning None for non-record lines and retain
app_version_is_validated as the sole classification fact.
In
`@crates/cmtraceopen-parser/src/intune/portal/windows/company_portal/logs/document.rs`:
- Around line 146-159: Update the coverage status construction in the document
parser so total_records == 0 is handled before the unreadable == 0 check and
does not report CompanyPortalCoverageStatus::Available. Preserve the existing
Available status only for non-empty content with no unreadable records, while
retaining ParseFailed for records that fail to parse.
In
`@crates/cmtraceopen-parser/src/intune/portal/windows/company_portal/logs/mod.rs`:
- Around line 48-53: Remove the `pub use grammar::*;` re-export from the logs
module’s public exports, while keeping the underlying `grammar` module private.
Leave the other public re-exports unchanged so only the grammar implementation
items stop being exposed.
In `@crates/cmtraceopen-parser/src/parser/mod.rs`:
- Around line 146-148: Update the Company Portal parsing flow around
ParserImplementation::CompanyPortal and ResolvedParser::company_portal() so
inventory_logical_dialect() includes Company Portal and its logical-record state
is persisted in TailReader. Ensure pending records are flushed at the next
header, debounce boundary, file rotation, and session shutdown, and add a
regression test covering a split write between header and continuation lines.
In `@crates/cmtraceopen-parser/tests/company_portal_windows_logs.rs`:
- Around line 700-732: Update
portal_logs_every_fixture_line_survives_into_a_record so each non-empty source
line is matched against at least one individual record.raw_text, rather than the
newline-joined joined string. Preserve the existing trimming and failure
context, but ensure matches cannot come from substrings in unrelated records or
across record boundaries.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f9eb6ef9-6271-4645-a44b-bb833e93ec63
⛔ Files ignored due to path filters (19)
Cargo.lockis excluded by!**/*.lock,!Cargo.lockcrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/code-tokens/Log_1.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/encoding-utf16le/Log_1.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/encoding-utf8-bom/Log_1.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/encoding-utf8-nobom/Log_1.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/invalid-timestamp/Log_1.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/malformed-structural-token/Log_1.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/multiline-continuation/Log_1.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/negative-generic-timestamped/Log_1.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/negative-unrelated-uwp/Log_1.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/redaction/Log_1.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/rotation/Log_1.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/rotation/Log_2.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/same-timestamp-distinct-activity/Log_1.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/severity-levels/Log_1.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/truncated-boundaries/Log_1.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v13-4-2/unknown-app-version/Log_1.logis excluded by!**/*.logsrc-tauri/tests/corpus/company_portal/clean/Log_1.logis excluded by!**/*.logsrc-tauri/tests/corpus/company_portal/negative/Log_1.logis excluded by!**/*.log
📒 Files selected for processing (22)
.gitattributes.github/workflows/cmtrace-ci.ymlcrates/cmtraceopen-parser/src/esp/mod.rscrates/cmtraceopen-parser/src/esp/redaction.rscrates/cmtraceopen-parser/src/intune/portal/windows/company_portal/logs/detect.rscrates/cmtraceopen-parser/src/intune/portal/windows/company_portal/logs/document.rscrates/cmtraceopen-parser/src/intune/portal/windows/company_portal/logs/entries.rscrates/cmtraceopen-parser/src/intune/portal/windows/company_portal/logs/framing.rscrates/cmtraceopen-parser/src/intune/portal/windows/company_portal/logs/grammar.rscrates/cmtraceopen-parser/src/intune/portal/windows/company_portal/logs/mod.rscrates/cmtraceopen-parser/src/intune/portal/windows/company_portal/logs/models.rscrates/cmtraceopen-parser/src/intune/portal/windows/company_portal/logs/redaction.rscrates/cmtraceopen-parser/src/models/log_entry.rscrates/cmtraceopen-parser/src/parser/detect.rscrates/cmtraceopen-parser/src/parser/mod.rscrates/cmtraceopen-parser/tests/company_portal_windows_logs.rsreferences/log-intune-reference.mdsrc-tauri/src/commands/bundle_ops.rssrc-tauri/tests/parser_supported_formats.rssrc/lib/column-config.tssrc/stores/log-store.tssrc/types/log.ts
Comment on lines
+146
to
+159
| coverage.push(CompanyPortalCoverage { | ||
| artifact_id: FILE_COVERAGE_ARTIFACT_ID.to_string(), | ||
| family: COVERAGE_FAMILY.to_string(), | ||
| status: if unreadable == 0 { | ||
| CompanyPortalCoverageStatus::Available | ||
| } else { | ||
| CompanyPortalCoverageStatus::ParseFailed | ||
| }, | ||
| detail: Some(format!( | ||
| "{} read {parsed_count} of {total_records} record(s) with grammar V1; \ | ||
| {unreadable} record(s) did not match and are preserved as source text.", | ||
| file.file_name | ||
| )), | ||
| }); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
An empty file reports Available coverage for zero records.
For empty or fully truncated content, total_records is 0, so unreadable == 0 and the file row claims Available with the detail "read 0 of 0 record(s)". A rotated file truncated to zero bytes is reachable. The document then asserts full coverage of an artifact from which nothing was read, which is the exact confusion the module doc says coverage exists to prevent. Branch on total_records == 0 first.
🐛 Proposed fix
- status: if unreadable == 0 {
+ status: if total_records == 0 {
+ // Nothing was read, so nothing is covered.
+ CompanyPortalCoverageStatus::Unsupported
+ } else if unreadable == 0 {
CompanyPortalCoverageStatus::Available
} else {
CompanyPortalCoverageStatus::ParseFailed
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| coverage.push(CompanyPortalCoverage { | |
| artifact_id: FILE_COVERAGE_ARTIFACT_ID.to_string(), | |
| family: COVERAGE_FAMILY.to_string(), | |
| status: if unreadable == 0 { | |
| CompanyPortalCoverageStatus::Available | |
| } else { | |
| CompanyPortalCoverageStatus::ParseFailed | |
| }, | |
| detail: Some(format!( | |
| "{} read {parsed_count} of {total_records} record(s) with grammar V1; \ | |
| {unreadable} record(s) did not match and are preserved as source text.", | |
| file.file_name | |
| )), | |
| }); | |
| coverage.push(CompanyPortalCoverage { | |
| artifact_id: FILE_COVERAGE_ARTIFACT_ID.to_string(), | |
| family: COVERAGE_FAMILY.to_string(), | |
| status: if total_records == 0 { | |
| // Nothing was read, so nothing is covered. | |
| CompanyPortalCoverageStatus::Unsupported | |
| } else if unreadable == 0 { | |
| CompanyPortalCoverageStatus::Available | |
| } else { | |
| CompanyPortalCoverageStatus::ParseFailed | |
| }, | |
| detail: Some(format!( | |
| "{} read {parsed_count} of {total_records} record(s) with grammar V1; \ | |
| {unreadable} record(s) did not match and are preserved as source text.", | |
| file.file_name | |
| )), | |
| }); |
🤖 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/intune/portal/windows/company_portal/logs/document.rs`
around lines 146 - 159, Update the coverage status construction in the document
parser so total_records == 0 is handled before the unreadable == 0 check and
does not report CompanyPortalCoverageStatus::Available. Preserve the existing
Available status only for non-empty content with no unreadable records, while
retaining ParseFailed for records that fail to parse.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Owner
Author
|
Correction checkpoint: What moved:
Exact-head local gates:
CI state:
|
Resolve the sole .gitattributes conflict by keeping byte-sensitive parser fixture attributes for Company Portal/SCCM corpora (-text -whitespace) and the Company Portal corpus path, so PR #460 is conflict-free against current main without force-pushing history.
adamgell
marked this pull request as ready for review
August 5, 2026 03:29
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Owner
Author
Lane A restack verification (Ivy)Restacked onto current Local gates (post-merge)
Conflict resolutionOnly Refs #366 |
…#460) - Drop always-true `is_record` from line classification (semver surface) - Stop re-exporting private grammar helpers; keep only `looks_like_record_start` - Match parse_state on borrowed kind arms in document builder - Correct ESP Windows job comment (parser tests also run on Linux check) - Strengthen lossless fixture test to exact line multiset membership Verified: company_portal_windows_logs (34) + logs unit tests (41) pass.
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 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/intune/portal/windows/company_portal/logs/mod.rs`:
- Around line 48-53: Preserve the public grammar exports in the logs module by
retaining the grammar glob re-export, including parse_record_fields,
leading_component, and CompanyPortalRecordFields. Do not replace or remove pub
use grammar::* in this 0.1.1 crate; defer any API cleanup to a planned breaking
release.
In
`@crates/cmtraceopen-parser/src/intune/portal/windows/company_portal/logs/models.rs`:
- Around line 25-30: Add #[non_exhaustive] to the public
CompanyPortalGrammarVersion and CompanyPortalTimestampKind enums, preserving
their existing derives, serde attributes, variants, and documentation.
In `@crates/cmtraceopen-parser/tests/company_portal_windows_logs.rs`:
- Around line 277-287: Update build_coverage so inputs producing zero parsed
records are assigned CompanyPortalCoverageStatus::ParseFailed rather than
Available. Preserve the existing coverage behavior for inputs with parsed
records, and ensure the empty document path used by
portal_logs_empty_input_is_not_available_coverage passes.
In `@src-tauri/src/commands/file_ops.rs`:
- Around line 711-727: Update index_aggregate_entries to index only tail-capable
Company Portal files, using the existing supported-file predicate or symbol.
When duplicate coordinates occur, skip or mark that file’s seed unavailable
instead of returning an AppError, so open_log_folder_aggregate continues
processing other files.
In `@src/lib/tail-payload-validation.test.ts`:
- Around line 248-267: The invalid optional-field table in the “rejects an
invalid optional LogEntry” test uses `{}` for every non-tags field, so it does
not validate each field’s constraints. Replace those placeholders with
field-appropriate invalid values, especially plausible unknown strings for
severity, format, and entryKind, and add explicit rejection cases covering
unknown values against the SEVERITIES, LOG_FORMATS, and ENTRY_KINDS-backed
fields while preserving the existing tags case.
- Around line 236-246: Update the assertion in the large-batch test “validates
large tail batches without spreading them into function arguments” to use
identity comparison with the original value instead of deep structural
comparison. Keep the existing parseTailPayload invocation and large-entry setup
unchanged.
In `@src/lib/tail-payload-validation.ts`:
- Around line 318-324: The payload validation branch around highestObservedLine
currently rejects the entire batch when observedThroughLine is null or lower
than the highest entry/amendment line. Replace that rejection with clamping
observedThroughLine to highestObservedLine and report the coverage
inconsistency, while preserving rejection for genuinely invalid state such as
out-of-range spans.
- Around line 161-166: Update physicalEndLine to count newline characters
without using message.split or allocating an array, then reuse the computed
physical end line from isLogEntry when parseTailPayload processes the same entry
instead of calling physicalEndLine again. Preserve the existing safe-integer
validation and null behavior.
- Around line 14-67: Replace the hand-maintained Set initializers LOG_FORMATS,
PARSER_KINDS, and PARSER_IMPLEMENTATIONS with exhaustive Record-based allowlists
keyed by each corresponding union, then derive each Set from the record keys.
Ensure adding a future variant to any union without updating its record causes
TypeScript compilation to fail while preserving the existing membership checks.
🪄 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: ef24fcf4-a31b-4a1e-a3b4-b26ea76b15af
⛔ Files ignored due to path filters (18)
crates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/code-tokens/Log_1.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/encoding-utf16le/Log_1.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/encoding-utf8-bom/Log_1.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/encoding-utf8-nobom/Log_1.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/invalid-timestamp/Log_1.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/malformed-structural-token/Log_1.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/multiline-continuation/Log_1.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/negative-generic-timestamped/Log_1.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/negative-unrelated-uwp/Log_1.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/redaction/Log_1.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/rotation/Log_1.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/rotation/Log_2.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/same-timestamp-distinct-activity/Log_1.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/severity-levels/Log_1.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/truncated-boundaries/Log_1.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v13-4-2/unknown-app-version/Log_1.logis excluded by!**/*.logsrc-tauri/tests/corpus/company_portal/clean/Log_1.logis excluded by!**/*.logsrc-tauri/tests/corpus/company_portal/negative/Log_1.logis excluded by!**/*.log
📒 Files selected for processing (31)
.gitattributes.github/workflows/cmtrace-ci.ymlcrates/cmtraceopen-parser/src/esp/mod.rscrates/cmtraceopen-parser/src/esp/redaction.rscrates/cmtraceopen-parser/src/intune/portal/windows/company_portal/logs/detect.rscrates/cmtraceopen-parser/src/intune/portal/windows/company_portal/logs/document.rscrates/cmtraceopen-parser/src/intune/portal/windows/company_portal/logs/entries.rscrates/cmtraceopen-parser/src/intune/portal/windows/company_portal/logs/framing.rscrates/cmtraceopen-parser/src/intune/portal/windows/company_portal/logs/grammar.rscrates/cmtraceopen-parser/src/intune/portal/windows/company_portal/logs/mod.rscrates/cmtraceopen-parser/src/intune/portal/windows/company_portal/logs/models.rscrates/cmtraceopen-parser/src/intune/portal/windows/company_portal/logs/redaction.rscrates/cmtraceopen-parser/src/models/log_entry.rscrates/cmtraceopen-parser/src/parser/detect.rscrates/cmtraceopen-parser/src/parser/mod.rscrates/cmtraceopen-parser/tests/company_portal_windows_logs.rsreferences/log-intune-reference.mdsrc-tauri/src/commands/bundle_ops.rssrc-tauri/src/commands/file_ops.rssrc-tauri/src/commands/parsing.rssrc-tauri/src/state/app_state.rssrc-tauri/src/watcher/tail.rssrc-tauri/tests/parser_supported_formats.rssrc/hooks/use-file-watcher.test.tsxsrc/hooks/use-file-watcher.tssrc/lib/column-config.tssrc/lib/tail-payload-validation.test.tssrc/lib/tail-payload-validation.tssrc/stores/log-store.test.tssrc/stores/log-store.tssrc/types/log.ts
Comment on lines
+48
to
+53
| pub use detect::*; | ||
| pub use document::*; | ||
| pub use entries::*; | ||
| pub use grammar::*; | ||
| pub use models::*; | ||
| pub use redaction::*; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Resolve every external reference to the grammar items published by the glob.
set -euo pipefail
logs_dir='crates/cmtraceopen-parser/src/intune/portal/windows/company_portal/logs'
echo '== public items in grammar.rs =='
ast-grep outline "$logs_dir/grammar.rs" --items all
echo '== references outside the logs module =='
rg -nP --type=rust -C2 \
'\b(parse_record_fields|looks_like_record_start|leading_component|CompanyPortalRecordFields)\b' \
-g "!$logs_dir/**"Repository: adamgell/cmtraceopen
Length of output: 2009
🏁 Script executed:
#!/bin/bash
set -euo pipefail
logs_dir='crates/cmtraceopen-parser/src/intune/portal/windows/company_portal/logs'
printf '%s\n' '== grammar public declarations =='
rg -n '^(pub(\([^)]*\))?[[:space:]]+(struct|enum|fn|type|const|static)|^pub[[:space:]]+use)' \
"$logs_dir/grammar.rs"
printf '%s\n' '== all grammar item references outside logs =='
rg -nP --type=rust -C1 \
'\b(parse_record_fields|looks_like_record_start|leading_component|CompanyPortalRecordFields)\b' \
-g "!$logs_dir/**" . || true
printf '%s\n' '== module re-exports and crate declarations =='
cat -n "$logs_dir/mod.rs"
rg -n 'cmtraceopen_parser|cmtraceopen-parser|frame_company_portal_logical_records|looks_like_record_start' \
--glob '*.rs' --glob '*.toml' .
printf '%s\n' '== relevant tail context =='
tail_file="$(rg --files | rg '(^|/)tail\.rs$' | head -n1)"
if [ -n "$tail_file" ]; then
sed -n '910,990p' "$tail_file"
else
echo 'tail.rs not found'
fiRepository: adamgell/cmtraceopen
Length of output: 38074
🏁 Script executed:
#!/bin/bash
set -euo pipefail
logs_dir='crates/cmtraceopen-parser/src/intune/portal/windows/company_portal/logs'
printf '%s\n' '== package version and repository state =='
rg -n '^(version|name)[[:space:]]*=' crates/cmtraceopen-parser/Cargo.toml
git status --short
git diff --numstat -- "$logs_dir/mod.rs" "$logs_dir/grammar.rs"
printf '%s\n' '== all Windows log test references to grammar exports =='
rg -n -C1 \
'\b(parse_record_fields|looks_like_record_start|leading_component|CompanyPortalRecordFields)\b|logs::\*' \
crates/cmtraceopen-parser/tests/company_portal_windows_logs.rs \
"$logs_dir"
printf '%s\n' '== public signatures that expose grammar types =='
rg -n -C2 \
'pub (fn|struct|enum|type|use)|CompanyPortalRecordFields|parse_record_fields|leading_component' \
"$logs_dir"/*.rsRepository: adamgell/cmtraceopen
Length of output: 48787
Preserve the public grammar exports. This 0.1.1 crate exposes parse_record_fields, leading_component, and CompanyPortalRecordFields through the glob. Replacing it removes public API items and causes a semver break. Keep the glob or defer removal to a planned breaking release.
🤖 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/intune/portal/windows/company_portal/logs/mod.rs`
around lines 48 - 53, Preserve the public grammar exports in the logs module by
retaining the grammar glob re-export, including parse_record_fields,
leading_component, and CompanyPortalRecordFields. Do not replace or remove pub
use grammar::* in this 0.1.1 crate; defer any API cleanup to a planned breaking
release.
Source: Path instructions
Comment on lines
+25
to
+30
| #[derive(Debug, Clone, Copy, Serialize, Deserialize, PartialEq, Eq)] | ||
| #[serde(rename_all = "camelCase")] | ||
| pub enum CompanyPortalGrammarVersion { | ||
| /// Field layout observed in app version `12-0-0`. | ||
| V1, | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Mark the version-scoped enums #[non_exhaustive].
CompanyPortalGrammarVersion and CompanyPortalTimestampKind each hold exactly one variant today, and both doc comments state that more variants are expected (V2 for a second observed layout, and the timestamp doc reserves the question of other resolutions). In a published crate, adding a variant later breaks every downstream exhaustive match. Add #[non_exhaustive] now, while the cost is zero.
♻️ Proposed change
#[derive(Debug, Clone, Copy, Serialize, Deserialize, PartialEq, Eq)]
#[serde(rename_all = "camelCase")]
+#[non_exhaustive]
pub enum CompanyPortalGrammarVersion {
/// Field layout observed in app version `12-0-0`.
V1,
} #[derive(Debug, Clone, Copy, Serialize, Deserialize, PartialEq, Eq)]
#[serde(rename_all = "camelCase")]
+#[non_exhaustive]
pub enum CompanyPortalTimestampKind {
/// Resolved to an absolute UTC instant.
Utc,
}As per path instructions: "Treat every public item as a semver commitment: flag breaking changes to public types, signatures, or enum variants".
Also applies to: 89-94
🤖 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/intune/portal/windows/company_portal/logs/models.rs`
around lines 25 - 30, Add #[non_exhaustive] to the public
CompanyPortalGrammarVersion and CompanyPortalTimestampKind enums, preserving
their existing derives, serde attributes, variants, and documentation.
Source: Path instructions
Comment on lines
+711
to
+727
| fn index_aggregate_entries( | ||
| entries: &[LogEntry], | ||
| ) -> Result<std::collections::HashMap<(&str, u32), &LogEntry>, crate::error::AppError> { | ||
| let mut lookup = std::collections::HashMap::new(); | ||
| for entry in entries { | ||
| if lookup | ||
| .insert((entry.file_path.as_str(), entry.line_number), entry) | ||
| .is_some() | ||
| { | ||
| return Err(crate::error::AppError::Internal(format!( | ||
| "duplicate aggregate entry for {} at physical line {}", | ||
| entry.file_path, entry.line_number | ||
| ))); | ||
| } | ||
| } | ||
| Ok(lookup) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Look for parsers that emit several entries for one physical line number.
set -uo pipefail
# Find parse_lines implementations and inspect how line_number is assigned.
fd -e rs . crates/cmtraceopen-parser/src --exec rg -nP -C4 'line_number\s*[:=]' {} \
| rg -n -C4 'line_number' \
| head -200
# Look for sites that push more than one entry inside a single line iteration.
ast-grep run --lang rust --pattern $'for $_ in $_ {
$$$
$ENTRIES.push($_);
$$$
$ENTRIES.push($_);
$$$
}' crates/cmtraceopen-parser/srcRepository: adamgell/cmtraceopen
Length of output: 8349
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- file_ops.rs relevant symbols ---'
rg -n -C12 'open_log_folder_aggregate|index_aggregate_entries|supports_parser|tail|Company Portal|aggregate' src-tauri/src/commands/file_ops.rs
printf '%s\n' '--- parser files ---'
fd -e rs . crates/cmtraceopen-parser/src | sort
printf '%s\n' '--- parser entry construction and aggregation ---'
rg -n -C8 'line_number_for_offset|LogEntry \{|entries\.push|parse_lines|Company Portal|company.?portal' crates/cmtraceopen-parser/src | head -500
printf '%s\n' '--- LogEntry and AppError definitions ---'
rg -n -C12 'struct LogEntry|enum AppError|type AppError' src-tauri/src crates/cmtraceopen-parser/srcRepository: adamgell/cmtraceopen
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- parser selection and tail-seed logic ---'
rg -n -C15 'enum ParserSelection|struct ParserSelection|supports_parser|from_entry|CompanyPortal|company_portal|WindowsCompanyPortal' \
crates/cmtraceopen-parser/src src-tauri/src/watcher src-tauri/src/commands/file_ops.rs
printf '%s\n' '--- parser entry points ---'
rg -n 'pub fn parse_lines|fn parse_lines|parse_lines\(' crates/cmtraceopen-parser/src/parser crates/cmtraceopen-parser/src/intune/portal/windows/company_portal
printf '%s\n' '--- all direct line-number assignments in parser source ---'
rg -n -P 'line_number\s*:\s*|\.line_number\s*=' crates/cmtraceopen-parser/src/parser crates/cmtraceopen-parser/src/intune/portal/windows/company_portal
printf '%s\n' '--- parser dispatch ---'
rg -n -C20 'parse_file|match.*parser|ParserKind|parser_selection' crates/cmtraceopen-parser/src src-tauri/src/parser.rs src-tauri/src/commands/file_ops.rsRepository: adamgell/cmtraceopen
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- tail seed implementation ---'
rg -n -C20 'struct InitialLogicalRecord|impl InitialLogicalRecord|supports_parser|from_entry|from_parse_result' \
src-tauri/src/watcher/tail.rs src-tauri/src/watcher
printf '%s\n' '--- parser dispatch ---'
sed -n '1,175p' crates/cmtraceopen-parser/src/parser/mod.rs
printf '%s\n' '--- Company Portal framing ---'
cat -n crates/cmtraceopen-parser/src/intune/portal/windows/company_portal/logs/framing.rs
printf '%s\n' '--- Company Portal document and entries ---'
cat -n crates/cmtraceopen-parser/src/intune/portal/windows/company_portal/logs/document.rs
cat -n crates/cmtraceopen-parser/src/intune/portal/windows/company_portal/logs/entries.rs
printf '%s\n' '--- parser selection model ---'
rg -n -C8 'pub enum ParserImplementation|pub struct ResolvedParser|CompanyPortal|CompanyPortalWindows|RecordFraming' \
crates/cmtraceopen-parser/src/models/log_entry.rs crates/cmtraceopen-parser/src/parser/mod.rsRepository: adamgell/cmtraceopen
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- Company Portal framing ---'
cat -n crates/cmtraceopen-parser/src/intune/portal/windows/company_portal/logs/framing.rs
printf '%s\n' '--- Company Portal document ---'
cat -n crates/cmtraceopen-parser/src/intune/portal/windows/company_portal/logs/document.rs
printf '%s\n' '--- Company Portal entry model and construction ---'
sed -n '1,180p' crates/cmtraceopen-parser/src/intune/portal/windows/company_portal/logs/entries.rs
sed -n '180,280p' crates/cmtraceopen-parser/src/intune/portal/windows/company_portal/logs/models.rs
printf '%s\n' '--- Company Portal parse function ---'
rg -n -C25 'pub fn parse_lines|fn parse_lines' \
crates/cmtraceopen-parser/src/intune/portal/windows/company_portal/logsRepository: adamgell/cmtraceopen
Length of output: 35770
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
root = Path("crates/cmtraceopen-parser/src")
framing = (root / "intune/portal/windows/company_portal/logs/framing.rs").read_text()
assert "pub fn frame_records" in framing
assert "(index + 1) as u32" in framing
assert framing.count("records.push(FramedRecord") == 3
assert "line_number: (index + 1) as u32" in framing
print("Company Portal framing: every record push uses the current physical line index")
for path in sorted(root.rglob("*.rs")):
text = path.read_text()
if "parse_lines" not in text:
continue
pushes = len(re.findall(r"\bentries\.push\s*\(", text))
assignments = len(re.findall(r"\bline_number\s*:", text))
if pushes:
print(f"{path}: entries.push={pushes}, line_number fields={assignments}")
PYRepository: adamgell/cmtraceopen
Length of output: 246
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
root = Path("crates/cmtraceopen-parser/src")
framing = (root / "intune/portal/windows/company_portal/logs/framing.rs").read_text()
checks = {
"frame_records exists": "frame_records<'a>" in framing,
"physical line assignment exists": "line_number: (index + 1) as u32" in framing,
"three mutually exclusive push branches present":
len(re.findall(r"\brecords\.push\s*\(\s*FramedRecord\s*\{", framing)) == 3,
}
for name, passed in checks.items():
print(f"{name}: {'PASS' if passed else 'FAIL'}")
assert all(checks.values())
print("Company Portal framing assigns each emitted record the current physical line number.")
for path in sorted(root.rglob("*.rs")):
text = path.read_text()
if "parse_lines" not in text:
continue
pushes = len(re.findall(r"\bentries\.push\s*\(", text))
if pushes:
print(f"{path}: entries.push={pushes}")
PYRepository: adamgell/cmtraceopen
Length of output: 1624
Scope the aggregate index to tail-capable files. The lookup is queried only for the final entry of Company Portal files, but it currently allocates a bucket for every aggregate entry. Restrict the index to supported files and let a duplicate coordinate disable that file’s seed instead of aborting open_log_folder_aggregate.
🤖 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/commands/file_ops.rs` around lines 711 - 727, Update
index_aggregate_entries to index only tail-capable Company Portal files, using
the existing supported-file predicate or symbol. When duplicate coordinates
occur, skip or mark that file’s seed unavailable instead of returning an
AppError, so open_log_folder_aggregate continues processing other files.
Comment on lines
+236
to
+246
| it("validates large tail batches without spreading them into function arguments", () => { | ||
| const entries = Array.from({ length: 200_000 }, (_, index) => | ||
| entry({ id: index, lineNumber: index + 1 }), | ||
| ); | ||
| const value = payload({ | ||
| entries, | ||
| observedThroughLine: entries.length, | ||
| }); | ||
|
|
||
| expect(parseTailPayload(value)).toEqual(value); | ||
| }); |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
Use toBe for the 200,000-entry batch.
parseTailPayload returns the same object reference it was given at Line 326 of src/lib/tail-payload-validation.ts. toEqual therefore performs a deep structural comparison of 200,000 objects to prove something identity already proves. toBe(value) asserts the same contract in constant time and keeps the suite fast.
♻️ Identity assertion
- expect(parseTailPayload(value)).toEqual(value);
+ expect(parseTailPayload(value)).toBe(value);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it("validates large tail batches without spreading them into function arguments", () => { | |
| const entries = Array.from({ length: 200_000 }, (_, index) => | |
| entry({ id: index, lineNumber: index + 1 }), | |
| ); | |
| const value = payload({ | |
| entries, | |
| observedThroughLine: entries.length, | |
| }); | |
| expect(parseTailPayload(value)).toEqual(value); | |
| }); | |
| it("validates large tail batches without spreading them into function arguments", () => { | |
| const entries = Array.from({ length: 200_000 }, (_, index) => | |
| entry({ id: index, lineNumber: index + 1 }), | |
| ); | |
| const value = payload({ | |
| entries, | |
| observedThroughLine: entries.length, | |
| }); | |
| expect(parseTailPayload(value)).toBe(value); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/tail-payload-validation.test.ts` around lines 236 - 246, Update the
assertion in the large-batch test “validates large tail batches without
spreading them into function arguments” to use identity comparison with the
original value instead of deep structural comparison. Keep the existing
parseTailPayload invocation and large-entry setup unchanged.
Comment on lines
+248
to
+267
| it.each( | ||
| Object.keys(validOptionalFields).map((field) => [ | ||
| field, | ||
| field === "tags" ? ["valid", 1] : {}, | ||
| ]), | ||
| )("rejects an invalid optional LogEntry %s", (field, invalidValue) => { | ||
| expect( | ||
| parseTailPayload( | ||
| payload({ | ||
| entries: [ | ||
| { | ||
| ...entry(), | ||
| [field]: invalidValue, | ||
| }, | ||
| ], | ||
| observedThroughLine: 1, | ||
| }), | ||
| ), | ||
| ).toBeNull(); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Every non-tags case uses {}, so this table proves only that an object is refused.
The invalid value at Line 251 is {} for all fields except tags. That triggers rejection for the same trivial reason in every case. It does not exercise the constraint each field actually declares.
The gap that matters is the string enums. severity, format, and entryKind gate every entry through SEVERITIES, LOG_FORMATS, and ENTRY_KINDS, and no test supplies a plausible wrong string such as "Bogus" or "Verbose". Those sets are hand-maintained mirrors of Rust enums, so a wrong-string case is the realistic failure and it is untested.
Add per-field invalid values that match each field's declared type, and add explicit rejection cases for an unknown severity, format, and entryKind.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/tail-payload-validation.test.ts` around lines 248 - 267, The invalid
optional-field table in the “rejects an invalid optional LogEntry” test uses
`{}` for every non-tags field, so it does not validate each field’s constraints.
Replace those placeholders with field-appropriate invalid values, especially
plausible unknown strings for severity, format, and entryKind, and add explicit
rejection cases covering unknown values against the SEVERITIES, LOG_FORMATS, and
ENTRY_KINDS-backed fields while preserving the existing tags case.
Comment on lines
+14
to
+67
| const LOG_FORMATS = new Set<LogFormat>([ | ||
| "Ccm", | ||
| "Simple", | ||
| "Plain", | ||
| "Timestamped", | ||
| "DnsDebug", | ||
| "DnsAudit", | ||
| "CmtLog", | ||
| ]); | ||
| const PARSER_KINDS = new Set<ParserKind>([ | ||
| "ccm", | ||
| "simple", | ||
| "timestamped", | ||
| "plain", | ||
| "iisW3c", | ||
| "panther", | ||
| "cbs", | ||
| "dism", | ||
| "reportingEvents", | ||
| "msi", | ||
| "psadtLegacy", | ||
| "intuneMacOs", | ||
| "intuneDeviceInventory", | ||
| "dhcp", | ||
| "burn", | ||
| "patchMyPcDetection", | ||
| "registry", | ||
| "secureBootLog", | ||
| "dnsDebug", | ||
| "dnsAudit", | ||
| "cmtLog", | ||
| "companyPortal", | ||
| ]); | ||
| const PARSER_IMPLEMENTATIONS = new Set<ParserImplementation>([ | ||
| "ccm", | ||
| "simple", | ||
| "genericTimestamped", | ||
| "iisW3c", | ||
| "reportingEvents", | ||
| "plainText", | ||
| "msi", | ||
| "psadtLegacy", | ||
| "intuneMacOs", | ||
| "intuneDeviceInventory", | ||
| "dhcp", | ||
| "burn", | ||
| "patchMyPcDetection", | ||
| "registry", | ||
| "secureBootLog", | ||
| "dnsDebug", | ||
| "dnsAudit", | ||
| "cmtLog", | ||
| "companyPortal", | ||
| ]); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Make these allowlists fail at compile time when a Rust variant is added.
LOG_FORMATS, PARSER_KINDS, and PARSER_IMPLEMENTATIONS are hand-maintained mirrors of Rust enums. The typed Set<ParserKind> annotation catches an entry that is not part of the union, but it cannot catch a missing entry. That is the failure direction that matters here.
If a future variant is added to ParserKind and not added at Line 23, isParserSelection rejects the payload, parseTailPayload returns null, and use-file-watcher.ts drops the batch with only a console.error. Tailing then stops for that parser with no visible cause. This PR already had to hand-add companyPortal in two places.
Build each set from an exhaustive record so npx tsc --noEmit fails when a variant is missing.
♻️ Compile-enforced allowlists
-const PARSER_KINDS = new Set<ParserKind>([
- "ccm",
- "simple",
+const PARSER_KIND_MEMBERS: Record<ParserKind, true> = {
+ ccm: true,
+ simple: true,
// ...every remaining variant, each required by the Record type
- "companyPortal",
-]);
+ companyPortal: true,
+};
+const PARSER_KINDS = new Set<ParserKind>(
+ Object.keys(PARSER_KIND_MEMBERS) as ParserKind[],
+);Apply the same shape to LOG_FORMATS and PARSER_IMPLEMENTATIONS.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/tail-payload-validation.ts` around lines 14 - 67, Replace the
hand-maintained Set initializers LOG_FORMATS, PARSER_KINDS, and
PARSER_IMPLEMENTATIONS with exhaustive Record-based allowlists keyed by each
corresponding union, then derive each Set from the record keys. Ensure adding a
future variant to any union without updating its record causes TypeScript
compilation to fail while preserving the existing membership checks.
Comment on lines
+161
to
+166
| function physicalEndLine(entry: LogEntry): number | null { | ||
| const endLine = entry.lineNumber + entry.message.split("\n").length - 1; | ||
| return isSafeInteger(endLine, entry.lineNumber, 4_294_967_295) | ||
| ? endLine | ||
| : null; | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
physicalEndLine splits every message twice and allocates an array each time.
isLogEntry calls physicalEndLine at Line 217, and parseTailPayload calls it again for the same entry at Line 301. Each call runs message.split("\n"), which allocates one array per message. The cost is two full passes over the message bytes of the whole batch, plus the garbage. The test at Line 236 of src/lib/tail-payload-validation.test.ts already exercises 200,000 entries.
Count newlines without allocating, and reuse the value computed during entry validation.
♻️ Allocation-free line count
function physicalEndLine(entry: LogEntry): number | null {
- const endLine = entry.lineNumber + entry.message.split("\n").length - 1;
+ let newlines = 0;
+ for (let index = 0; index < entry.message.length; index += 1) {
+ if (entry.message.charCodeAt(index) === 10) {
+ newlines += 1;
+ }
+ }
+ const endLine = entry.lineNumber + newlines;
return isSafeInteger(endLine, entry.lineNumber, 4_294_967_295)
? endLine
: null;
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/tail-payload-validation.ts` around lines 161 - 166, Update
physicalEndLine to count newline characters without using message.split or
allocating an array, then reuse the computed physical end line from isLogEntry
when parseTailPayload processes the same entry instead of calling
physicalEndLine again. Preserve the existing safe-integer validation and null
behavior.
Comment on lines
+318
to
+324
| if ( | ||
| highestObservedLine !== null && | ||
| (payload.observedThroughLine === null || | ||
| payload.observedThroughLine < highestObservedLine) | ||
| ) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
A low observedThroughLine discards the whole batch, including valid entries.
This check rejects the entire payload when observedThroughLine is below the highest line implied by entries or amendments. The caller then logs one line and drops every entry in the batch.
That couples a backend coverage-accounting mistake to total data loss in the UI. The physical_lines under-count I flagged at src-tauri/src/watcher/tail.rs Lines 955 to 966 produces exactly this shape: parse_logical_records derives observed_through_line from next_line, so an under-count makes the reported coverage fall below an entry's own end line and this branch drops records the backend parsed correctly.
Consider clamping observedThroughLine up to highestObservedLine and reporting the inconsistency, rather than discarding parsed entries. Reserve rejection for fields that would corrupt state, such as out-of-range spans.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/tail-payload-validation.ts` around lines 318 - 324, The payload
validation branch around highestObservedLine currently rejects the entire batch
when observedThroughLine is null or lower than the highest entry/amendment line.
Replace that rejection with clamping observedThroughLine to highestObservedLine
and report the coverage inconsistency, while preserving rejection for genuinely
invalid state such as out-of-range spans.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
crates/cmtraceopen-parser/src/intune/portal/windows/company_portal/logs/mod.rs (1)
51-53:⚠️ Potential issue | 🟠 MajorRestore the previous public grammar exports.
This re-export keeps only
looks_like_record_start. It removes the previously exportedparse_record_fields,leading_component, andCompanyPortalRecordFields. Downstream crates that use these items will fail to compile. Keep the existing exports, or defer the removal to a planned breaking release.As per path instructions: "Treat every public item as a semver commitment."
🤖 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/intune/portal/windows/company_portal/logs/mod.rs` around lines 51 - 53, Restore the public re-exports for parse_record_fields, leading_component, and CompanyPortalRecordFields alongside looks_like_record_start in the grammar exports of the logs module. Preserve downstream access to all previously public items and do not make their removal part of this change.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/cmtraceopen-parser/tests/company_portal_windows_logs.rs`:
- Around line 713-715: After the source-line matching loop in the lossless
matrix test, assert that remaining is empty so parsed output cannot contain
extra or duplicated lines; preserve the existing matching and multiplicity
checks.
---
Duplicate comments:
In
`@crates/cmtraceopen-parser/src/intune/portal/windows/company_portal/logs/mod.rs`:
- Around line 51-53: Restore the public re-exports for parse_record_fields,
leading_component, and CompanyPortalRecordFields alongside
looks_like_record_start in the grammar exports of the logs module. Preserve
downstream access to all previously public items and do not make their removal
part of this change.
🪄 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: 2d853e0f-8b37-48b9-9e22-fd1f93dae999
📒 Files selected for processing (5)
.github/workflows/cmtrace-ci.ymlcrates/cmtraceopen-parser/src/intune/portal/windows/company_portal/logs/detect.rscrates/cmtraceopen-parser/src/intune/portal/windows/company_portal/logs/document.rscrates/cmtraceopen-parser/src/intune/portal/windows/company_portal/logs/mod.rscrates/cmtraceopen-parser/tests/company_portal_windows_logs.rs
Comment on lines
+713
to
+715
| // Lossless-by-construction check across the whole matrix: every non-empty | ||
| // source line must appear as a complete framed line (not merely as a | ||
| // substring of some other line), and multiplicity is preserved. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert that no parsed lines remain unmatched.
The loop verifies that every source line exists in the parsed output. It does not verify that the parsed output contains only those lines. Extra or duplicated parsed lines remain in remaining, so the test can pass while multiplicity is incorrect. Assert that remaining is empty after the loop.
Proposed assertion
remaining.remove(index);
}
+ assert!(
+ remaining.is_empty(),
+ "{label}: parser emitted extra or duplicated lines: {remaining:?}"
+ );Also applies to: 731-744
🤖 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/company_portal_windows_logs.rs` around lines
713 - 715, After the source-line matching loop in the lossless matrix test,
assert that remaining is empty so parsed output cannot contain extra or
duplicated lines; preserve the existing matching and multiplicity checks.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/cmtrace-ci.yml (1)
233-247: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winThe
restore-keysfallback can serve a stalesrc-tauri/target/to this job.Line 243 restores any
${{ runner.os }}-esp-cargo-cache when the lockfile hash changes. On Windows a target directory built by a different toolchain revision produces loader failures at test start, which matches theSTATUS_ENTRYPOINT_NOT_FOUNDthis job reported twice before any test ran. Include the toolchain identity in the key, or cache only the registry and git directories and rebuildtarget/on this job.♻️ Suggested key change
- key: ${{ runner.os }}-esp-cargo-${{ hashFiles('Cargo.lock') }} - restore-keys: ${{ runner.os }}-esp-cargo- + key: ${{ runner.os }}-esp-cargo-${{ steps.toolchain.outputs.cachekey }}-${{ hashFiles('Cargo.lock') }} + restore-keys: ${{ runner.os }}-esp-cargo-${{ steps.toolchain.outputs.cachekey }}-
dtolnay/rust-toolchainexposescachekey; give the step at Line 231 anid: toolchain.🤖 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 @.github/workflows/cmtrace-ci.yml around lines 233 - 247, Update the “Cache Rust dependencies” step and its surrounding toolchain setup to prevent restore-keys from reusing an incompatible src-tauri/target directory: expose the dtolnay/rust-toolchain cachekey with the toolchain step id “toolchain” and incorporate that identity into the cache key and restore-keys, or remove target/ from the cached paths while retaining registry and git caches.
♻️ Duplicate comments (6)
crates/cmtraceopen-parser/tests/company_portal_windows_logs.rs (2)
739-746: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winStill open: assert that
remainingis empty after the loop.The multiset now proves every source line survives. It does not prove the parser emitted nothing extra. A record that duplicates a physical line, or that invents one, leaves entries in
remainingand the test still passes. Multiplicity is only half checked.💚 Proposed assertion
remaining.remove(index); } + assert!( + remaining.is_empty(), + "{label}: parser emitted extra or duplicated lines: {remaining:?}" + ); }As per path instructions: "Verify assertions test real behavior rather than restating the implementation."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/cmtraceopen-parser/tests/company_portal_windows_logs.rs` around lines 739 - 746, After the loop that matches each non-empty source line against remaining, assert that remaining is empty so extra or duplicated parser output causes the test to fail. Keep the existing multiplicity-aware matching and loss panic unchanged, and add the assertion in the same test flow.Source: Path instructions
277-287: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winThis test fails on this head, and it panics on the wrong assertion when it does.
The PR objectives state that the empty-input correction was not applied to this head, so
build_coveragestill reportsAvailablefor zero records. Fixbuild_coverageso zero parsed records cannot yield available coverage.Second, separate defect in the test itself: Line 283 indexes
coverage[0]with no prior length check. If the empty path yields an empty coverage vector, the failure is an index-out-of-bounds panic rather than the contract message on Line 285. Assert the vector is non-empty first.💚 Proposed test hardening
assert!(document.records.is_empty()); + assert!( + !document.coverage.is_empty(), + "an empty input must still produce a coverage row" + ); assert_eq!( document.coverage[0].status,🤖 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/company_portal_windows_logs.rs` around lines 277 - 287, Update build_coverage so inputs producing zero parsed records never receive CompanyPortalCoverageStatus::Available, while preserving the existing non-empty coverage behavior. In portal_logs_empty_input_is_not_available_coverage, first assert document.coverage is non-empty, then access coverage[0] for the status assertion so failures report the intended contract violation instead of panicking.src/lib/tail-payload-validation.test.ts (1)
248-267: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winEvery non-tags case passes
{}, so this table proves only that an object is refused.Line 251 supplies
{}for all fields excepttags. That trips rejection for the same trivial reason each time and never exercises the constraint the field declares.The untested gap is the string enums.
severity,format, andentryKindgate every entry throughSEVERITIES,LOG_FORMATS, andENTRY_KINDS. No case supplies a plausible wrong string such as"Verbose"or"Bogus". Those sets are hand-maintained mirrors of Rust enums, which is the drift risk I flagged atsrc/lib/tail-payload-validation.tslines 14 to 67, so the realistic failure is the one with no coverage.Give each field an invalid value that matches its declared type, and add explicit unknown-string cases for
severity,format, andentryKind.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/tail-payload-validation.test.ts` around lines 248 - 267, Update the invalid optional-field table in the `rejects an invalid optional LogEntry` test so each field uses a type-compatible value that violates its specific constraint instead of `{}`. Add explicit unknown-string cases for `severity`, `format`, and `entryKind`, while retaining the existing `tags` case, so validation against `SEVERITIES`, `LOG_FORMATS`, and `ENTRY_KINDS` is exercised.src/lib/tail-payload-validation.ts (2)
161-166: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
physicalEndLinesplits every message twice and allocates one array per call.
isLogEntrycalls it at line 217 andparseTailPayloadcalls it again for the same entry at line 301. Each call runsmessage.split("\n"). That is two full passes over the message bytes of the whole batch plus 2N throwaway arrays, on the event thread. The test at line 236 ofsrc/lib/tail-payload-validation.test.tsalready drives 200,000 entries through this.Count newlines in place, and reuse the value across both call sites.
♻️ Allocation-free line count
function physicalEndLine(entry: LogEntry): number | null { - const endLine = entry.lineNumber + entry.message.split("\n").length - 1; + let newlines = 0; + for (let index = 0; index < entry.message.length; index += 1) { + if (entry.message.charCodeAt(index) === 10) { + newlines += 1; + } + } + const endLine = entry.lineNumber + newlines; return isSafeInteger(endLine, entry.lineNumber, 4_294_967_295) ? endLine : null; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/tail-payload-validation.ts` around lines 161 - 166, Update physicalEndLine and the validation flow to count newline characters without message.split allocations, then compute the physical end line from that count. Ensure isLogEntry and parseTailPayload reuse the same computed end-line value for each entry rather than invoking physicalEndLine twice, while preserving the existing integer-range validation and null behavior.
14-67: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winA missing enum entry silently stops tailing, and the type annotation cannot catch it.
LOG_FORMATS,PARSER_KINDS, andPARSER_IMPLEMENTATIONSmirror Rust enums by hand.Set<ParserKind>rejects an entry outside the union, but it accepts a set that is missing a variant. That is the direction that breaks.Add a variant to
ParserKindwithout adding it at line 23, andisParserSelectionfails,parseTailPayloadreturnsnull, anduse-file-watcher.tsline 122 drops the batch with oneconsole.error. Tailing stops with no visible cause.Derive each set from an exhaustive
Record<Union, true>sonpx tsc --noEmitfails on a missing variant.♻️ Compile-enforced allowlists
-const PARSER_KINDS = new Set<ParserKind>([ - "ccm", - "simple", +const PARSER_KIND_MEMBERS: Record<ParserKind, true> = { + ccm: true, + simple: true, // every remaining variant, each required by the Record type - "companyPortal", -]); + companyPortal: true, +}; +const PARSER_KINDS = new Set<ParserKind>( + Object.keys(PARSER_KIND_MEMBERS) as ParserKind[], +);Apply the same shape to
LOG_FORMATSandPARSER_IMPLEMENTATIONS.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/tail-payload-validation.ts` around lines 14 - 67, Replace the manually typed Set allowlists LOG_FORMATS, PARSER_KINDS, and PARSER_IMPLEMENTATIONS with exhaustive Record<Union, true> definitions, then derive each Set from its record keys. Ensure every current variant remains included and future additions to LogFormat, ParserKind, or ParserImplementation cause a TypeScript compile error until added to the corresponding allowlist.Source: Coding guidelines
src-tauri/src/commands/file_ops.rs (1)
711-727: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winA duplicate coordinate in any file aborts the whole folder load.
index_aggregate_entriesindexes every aggregate entry, but line 414 queries it only for Company Portal files. Line 399 propagates the duplicate error, soopen_log_folder_aggregatereturnsAppError::Internaland the user sees nothing. The folder can contain any parser, and a parser that emits two entries for one physical line makes an unrelated file's tail seed break the whole open.Scope the index to tail-capable files, and downgrade a duplicate to "no seed for this file".
🤖 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/commands/file_ops.rs` around lines 711 - 727, Update index_aggregate_entries and its caller in open_log_folder_aggregate to index only tail-capable files, such as Company Portal entries, before querying the index. Treat duplicate coordinates within that scoped index as an absent seed for the affected file rather than returning AppError::Internal, while preserving valid seed lookups and allowing unrelated parser entries to load normally.
🤖 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 @.gitattributes:
- Around line 7-14: Add the -whitespace attribute to the existing
src-tauri/tests/fixtures/** rule in .gitattributes, preserving its -text
behavior so deliberate CRLF fixture data is excluded from git diff --check
whitespace validation.
In @.github/workflows/cmtrace-ci.yml:
- Around line 249-250: Remove the redundant Windows-specific parser clippy step
while retaining the Windows parser test step for path-handling coverage. Rename
the relevant parser test step to “Parser crate tests” and the remaining parser
clippy step to “Parser crate clippy” in the workflow.
In `@crates/cmtraceopen-parser/src/models/log_entry.rs`:
- Line 70: Add rustdoc comments to the new public `ParserKind::CompanyPortal`
and `ParserImplementation::CompanyPortal` enum variants, clearly documenting
their purpose and behavior while leaving the existing enum definitions
unchanged.
In `@crates/cmtraceopen-parser/tests/company_portal_windows_logs.rs`:
- Around line 129-134: Update the continuation-line assertions in the MULTILINE
test to compare each expected physical line with an individual line from
record.raw_text, using whole-line equality rather than contains. Preserve
trimming only as currently intended and ensure the assertion rejects cross-line
matches and substring matches, validating the test’s byte-identity behavior.
In `@references/log-intune-reference.md`:
- Line 212: Update the MDM diagnostic export entry in the reference table to use
the actual filesystem path C:\Users\Public\Documents\MDMDiagnostics\ instead of
the Explorer display-name path, while preserving the existing format and export
instructions.
In `@src/stores/log-store.ts`:
- Around line 834-861: In amendEntry, replace the full buildGuidNameMap(entries)
rebuild with mergeGuidNameMap using the amended entry and existing guidNameMap.
Preserve the current entries update and ensure the incremental merge reflects
the single changed record equivalently.
---
Outside diff comments:
In @.github/workflows/cmtrace-ci.yml:
- Around line 233-247: Update the “Cache Rust dependencies” step and its
surrounding toolchain setup to prevent restore-keys from reusing an incompatible
src-tauri/target directory: expose the dtolnay/rust-toolchain cachekey with the
toolchain step id “toolchain” and incorporate that identity into the cache key
and restore-keys, or remove target/ from the cached paths while retaining
registry and git caches.
---
Duplicate comments:
In `@crates/cmtraceopen-parser/tests/company_portal_windows_logs.rs`:
- Around line 739-746: After the loop that matches each non-empty source line
against remaining, assert that remaining is empty so extra or duplicated parser
output causes the test to fail. Keep the existing multiplicity-aware matching
and loss panic unchanged, and add the assertion in the same test flow.
- Around line 277-287: Update build_coverage so inputs producing zero parsed
records never receive CompanyPortalCoverageStatus::Available, while preserving
the existing non-empty coverage behavior. In
portal_logs_empty_input_is_not_available_coverage, first assert
document.coverage is non-empty, then access coverage[0] for the status assertion
so failures report the intended contract violation instead of panicking.
In `@src-tauri/src/commands/file_ops.rs`:
- Around line 711-727: Update index_aggregate_entries and its caller in
open_log_folder_aggregate to index only tail-capable files, such as Company
Portal entries, before querying the index. Treat duplicate coordinates within
that scoped index as an absent seed for the affected file rather than returning
AppError::Internal, while preserving valid seed lookups and allowing unrelated
parser entries to load normally.
In `@src/lib/tail-payload-validation.test.ts`:
- Around line 248-267: Update the invalid optional-field table in the `rejects
an invalid optional LogEntry` test so each field uses a type-compatible value
that violates its specific constraint instead of `{}`. Add explicit
unknown-string cases for `severity`, `format`, and `entryKind`, while retaining
the existing `tags` case, so validation against `SEVERITIES`, `LOG_FORMATS`, and
`ENTRY_KINDS` is exercised.
In `@src/lib/tail-payload-validation.ts`:
- Around line 161-166: Update physicalEndLine and the validation flow to count
newline characters without message.split allocations, then compute the physical
end line from that count. Ensure isLogEntry and parseTailPayload reuse the same
computed end-line value for each entry rather than invoking physicalEndLine
twice, while preserving the existing integer-range validation and null behavior.
- Around line 14-67: Replace the manually typed Set allowlists LOG_FORMATS,
PARSER_KINDS, and PARSER_IMPLEMENTATIONS with exhaustive Record<Union, true>
definitions, then derive each Set from its record keys. Ensure every current
variant remains included and future additions to LogFormat, ParserKind, or
ParserImplementation cause a TypeScript compile error until added to the
corresponding allowlist.
🪄 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: f51a2302-a170-4638-b463-58530bbd3947
⛔ Files ignored due to path filters (18)
crates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/code-tokens/Log_1.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/encoding-utf16le/Log_1.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/encoding-utf8-bom/Log_1.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/encoding-utf8-nobom/Log_1.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/invalid-timestamp/Log_1.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/malformed-structural-token/Log_1.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/multiline-continuation/Log_1.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/negative-generic-timestamped/Log_1.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/negative-unrelated-uwp/Log_1.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/redaction/Log_1.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/rotation/Log_1.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/rotation/Log_2.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/same-timestamp-distinct-activity/Log_1.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/severity-levels/Log_1.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/truncated-boundaries/Log_1.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v13-4-2/unknown-app-version/Log_1.logis excluded by!**/*.logsrc-tauri/tests/corpus/company_portal/clean/Log_1.logis excluded by!**/*.logsrc-tauri/tests/corpus/company_portal/negative/Log_1.logis excluded by!**/*.log
📒 Files selected for processing (31)
.gitattributes.github/workflows/cmtrace-ci.ymlcrates/cmtraceopen-parser/src/esp/mod.rscrates/cmtraceopen-parser/src/esp/redaction.rscrates/cmtraceopen-parser/src/intune/portal/windows/company_portal/logs/detect.rscrates/cmtraceopen-parser/src/intune/portal/windows/company_portal/logs/document.rscrates/cmtraceopen-parser/src/intune/portal/windows/company_portal/logs/entries.rscrates/cmtraceopen-parser/src/intune/portal/windows/company_portal/logs/framing.rscrates/cmtraceopen-parser/src/intune/portal/windows/company_portal/logs/grammar.rscrates/cmtraceopen-parser/src/intune/portal/windows/company_portal/logs/mod.rscrates/cmtraceopen-parser/src/intune/portal/windows/company_portal/logs/models.rscrates/cmtraceopen-parser/src/intune/portal/windows/company_portal/logs/redaction.rscrates/cmtraceopen-parser/src/models/log_entry.rscrates/cmtraceopen-parser/src/parser/detect.rscrates/cmtraceopen-parser/src/parser/mod.rscrates/cmtraceopen-parser/tests/company_portal_windows_logs.rsreferences/log-intune-reference.mdsrc-tauri/src/commands/bundle_ops.rssrc-tauri/src/commands/file_ops.rssrc-tauri/src/commands/parsing.rssrc-tauri/src/state/app_state.rssrc-tauri/src/watcher/tail.rssrc-tauri/tests/parser_supported_formats.rssrc/hooks/use-file-watcher.test.tsxsrc/hooks/use-file-watcher.tssrc/lib/column-config.tssrc/lib/tail-payload-validation.test.tssrc/lib/tail-payload-validation.tssrc/stores/log-store.test.tssrc/stores/log-store.tssrc/types/log.ts
Comment on lines
7
to
+14
| src-tauri/tests/fixtures/** -text | ||
|
|
||
| # SCCM parser fixtures are byte-sensitive evidence; never normalize them. | ||
| crates/cmtraceopen-parser/tests/fixtures/** -text | ||
| # Parser-crate fixtures encode BOM, CRLF, and UTF-16LE deliberately; the tests | ||
| # assert on the decoded bytes, so a checkout must not rewrite line endings and | ||
| # `git diff --check` must not read a deliberate CR as trailing whitespace. | ||
| # Covers Company Portal, SCCM, and other pure-parser evidence corpora. | ||
| crates/cmtraceopen-parser/tests/fixtures/** -text -whitespace | ||
| src-tauri/tests/corpus/company_portal/** -text -whitespace |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the corpus path referenced by .gitattributes exists and holds fixtures.
fd -t d -H 'company_portal' src-tauri || echo "no company_portal dir under src-tauri"
fd -t d -H 'corpus' src-tauri || echo "no corpus dir under src-tauri"
fd -t f . src-tauri/tests 2>/dev/null | head -50
# Show which fixture paths the parser tests actually load.
rg -n 'include_(str|bytes)!' crates/cmtraceopen-parser/tests | head -50Repository: adamgell/cmtraceopen
Length of output: 10130
🏁 Script executed:
#!/bin/bash
printf '%s\n' '--- .gitattributes ---'
cat -n .gitattributes
printf '%s\n' '--- legacy fixture files and byte markers ---'
git ls-files 'src-tauri/tests/fixtures/**' 'src-tauri/tests/corpus/company_portal/**' |
while IFS= read -r f; do
printf '%s ' "$f"
python3 - "$f" <<'PY'
import sys
p = sys.argv[1]
b = open(p, "rb").read()
print({"bytes": len(b), "cr": b.count(b"\r"), "lf": b.count(b"\n"), "utf16le_bom": b.startswith(b"\xff\xfe"), "utf8_bom": b.startswith(b"\xef\xbb\xbf")})
PY
done
printf '%s\n' '--- effective attributes ---'
git check-attr -a -- \
src-tauri/tests/fixtures/ccm/psadt_install.log \
src-tauri/tests/corpus/company_portal/clean/Log_1.log \
crates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/encoding-utf16le/Log_1.log
printf '%s\n' '--- whitespace check for legacy fixtures ---'
git diff --check --no-index /dev/null src-tauri/tests/fixtures/ccm/psadt_install.log || true
git diff --check --no-index /dev/null src-tauri/tests/corpus/company_portal/clean/Log_1.log || trueRepository: adamgell/cmtraceopen
Length of output: 24113
Add -whitespace to src-tauri/tests/fixtures/**.
These fixtures contain CRLF data, and git diff --check reports the deliberate CR bytes as trailing whitespace.
🤖 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 @.gitattributes around lines 7 - 14, Add the -whitespace attribute to the
existing src-tauri/tests/fixtures/** rule in .gitattributes, preserving its
-text behavior so deliberate CRLF fixture data is excluded from git diff --check
whitespace validation.
Comment on lines
+249
to
+250
| - name: Clippy parser crate | ||
| run: cargo clippy --locked -p cmtraceopen-parser --all-targets -- -D warnings |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Does the parser crate contain any Windows-gated code that clippy would only lint on Windows?
rg -n 'cfg\(target_os *= *"windows"\)|cfg\(windows\)|target_family *= *"windows"' crates/cmtraceopen-parser || echo "no windows-gated code in cmtraceopen-parser"
rg -n -A20 '^\[(target|dependencies|features)' crates/cmtraceopen-parser/Cargo.tomlRepository: adamgell/cmtraceopen
Length of output: 480
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- workflow context ---'
sed -n '60,100p;225,260p' .github/workflows/cmtrace-ci.yml
printf '%s\n' '--- parser crate source files and cfg attributes ---'
git ls-files crates/cmtraceopen-parser
rg -n 'cfg|target_os|target_family|windows|winapi|windows-sys' crates/cmtraceopen-parser || true
printf '%s\n' '--- workspace and package metadata ---'
sed -n '1,80p' crates/cmtraceopen-parser/Cargo.toml
rg -n -A12 -B3 'cmtraceopen-parser|workspace|target\.' Cargo.toml crates .github/workflows/cmtrace-ci.ymlRepository: adamgell/cmtraceopen
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
import re
workflow = Path(".github/workflows/cmtrace-ci.yml").read_text()
manifest = Path("crates/cmtraceopen-parser/Cargo.toml").read_text()
sources = [
p for p in Path("crates/cmtraceopen-parser").rglob("*")
if p.is_file() and p.suffix in {".rs", ".toml"}
]
windows_cfg = []
for path in sources:
text = path.read_text(errors="replace")
if re.search(r'#\s*\[\s*cfg\s*\([^]]*(?:windows|target_os|target_family)[^]]*\)', text):
windows_cfg.append(str(path))
target_sections = re.findall(
r'(?ms)^\[target[^\]]*\].*?(?=^\[|\Z)', manifest
)
commands = re.findall(
r'(?ms)^\s*- name: (Parser crate tests|Parser crate clippy|Test parser crate contracts|Clippy parser crate)\s*$.*?^\s+run: (.+)$',
workflow,
)
print("windows_cfg_files:", windows_cfg or "none")
print("target_manifest_sections:", len(target_sections))
print("parser_commands:")
for name, command in commands:
print(f" {name}: {command}")
print("linux_command_count:", workflow.count(
"cargo clippy --locked -p cmtraceopen-parser --all-targets -- -D warnings"
))
PYRepository: adamgell/cmtraceopen
Length of output: 10627
Remove the redundant Windows parser clippy step. The parser crate has no Windows-gated source or target-specific manifest sections. Keep the Windows test step for Windows path handling. Rename the steps to Parser crate tests and Parser crate clippy.
🤖 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 @.github/workflows/cmtrace-ci.yml around lines 249 - 250, Remove the
redundant Windows-specific parser clippy step while retaining the Windows parser
test step for path-handling coverage. Rename the relevant parser test step to
“Parser crate tests” and the remaining parser clippy step to “Parser crate
clippy” in the workflow.
| DnsDebug, | ||
| DnsAudit, | ||
| CmtLog, | ||
| CompanyPortal, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document the new public enum variants.
Add rustdoc comments for ParserKind::CompanyPortal and ParserImplementation::CompanyPortal. These variants are public API commitments.
As per path instructions: "Treat every public item as a semver commitment: flag breaking changes to public types, signatures, or enum variants, and check that new public items are documented."
Also applies to: 95-95
🤖 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/models/log_entry.rs` at line 70, Add rustdoc
comments to the new public `ParserKind::CompanyPortal` and
`ParserImplementation::CompanyPortal` enum variants, clearly documenting their
purpose and behavior while leaving the existing enum definitions unchanged.
Source: Path instructions
Comment on lines
+834
to
+861
| amendEntry: (amendment) => { | ||
| let amendmentApplied = false; | ||
| set((state) => { | ||
| const entryIndex = state.entries.findIndex( | ||
| (entry) => | ||
| entry.id === amendment.entryId && | ||
| canApplyTailAmendment(entry, amendment), | ||
| ); | ||
| if (entryIndex < 0) { | ||
| return state; | ||
| } | ||
|
|
||
| amendmentApplied = true; | ||
| const entries = [...state.entries]; | ||
| entries[entryIndex] = applyTailAmendment(entries[entryIndex], amendment); | ||
| return { | ||
| entries, | ||
| totalLines: Math.max( | ||
| state.totalLines, | ||
| amendment.continuationEndLine, | ||
| ), | ||
| guidNameMap: buildGuidNameMap(entries), | ||
| }; | ||
| }); | ||
| if (amendmentApplied) { | ||
| recomputeAndSetMatches(); | ||
| } | ||
| }, |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Replace the full buildGuidNameMap rebuild with the incremental merge.
Line 855 rebuilds the GUID map from every entry for a single amended record. buildGuidNameMap walks all messages and parses JSON for each "Get policies" hit. Amendments arrive per tail batch on Company Portal continuation lines, so this runs repeatedly against a list that can hold hundreds of thousands of entries.
appendEntries (Line 830) and amendAggregateEntry (Line 963) already use the incremental mergeGuidNameMap. Use it here too. The result is equivalent because only one entry's message changed.
♻️ Proposed change
amendmentApplied = true;
const entries = [...state.entries];
- entries[entryIndex] = applyTailAmendment(entries[entryIndex], amendment);
+ const amendedEntry = applyTailAmendment(entries[entryIndex], amendment);
+ entries[entryIndex] = amendedEntry;
return {
entries,
totalLines: Math.max(
state.totalLines,
amendment.continuationEndLine,
),
- guidNameMap: buildGuidNameMap(entries),
+ guidNameMap: mergeGuidNameMap(state.guidNameMap, [amendedEntry]),
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| amendEntry: (amendment) => { | |
| let amendmentApplied = false; | |
| set((state) => { | |
| const entryIndex = state.entries.findIndex( | |
| (entry) => | |
| entry.id === amendment.entryId && | |
| canApplyTailAmendment(entry, amendment), | |
| ); | |
| if (entryIndex < 0) { | |
| return state; | |
| } | |
| amendmentApplied = true; | |
| const entries = [...state.entries]; | |
| entries[entryIndex] = applyTailAmendment(entries[entryIndex], amendment); | |
| return { | |
| entries, | |
| totalLines: Math.max( | |
| state.totalLines, | |
| amendment.continuationEndLine, | |
| ), | |
| guidNameMap: buildGuidNameMap(entries), | |
| }; | |
| }); | |
| if (amendmentApplied) { | |
| recomputeAndSetMatches(); | |
| } | |
| }, | |
| amendEntry: (amendment) => { | |
| let amendmentApplied = false; | |
| set((state) => { | |
| const entryIndex = state.entries.findIndex( | |
| (entry) => | |
| entry.id === amendment.entryId && | |
| canApplyTailAmendment(entry, amendment), | |
| ); | |
| if (entryIndex < 0) { | |
| return state; | |
| } | |
| amendmentApplied = true; | |
| const entries = [...state.entries]; | |
| const amendedEntry = applyTailAmendment(entries[entryIndex], amendment); | |
| entries[entryIndex] = amendedEntry; | |
| return { | |
| entries, | |
| totalLines: Math.max( | |
| state.totalLines, | |
| amendment.continuationEndLine, | |
| ), | |
| guidNameMap: mergeGuidNameMap(state.guidNameMap, [amendedEntry]), | |
| }; | |
| }); | |
| if (amendmentApplied) { | |
| recomputeAndSetMatches(); | |
| } | |
| }, |
🤖 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/stores/log-store.ts` around lines 834 - 861, In amendEntry, replace the
full buildGuidNameMap(entries) rebuild with mergeGuidNameMap using the amended
entry and existing guidNameMap. Preserve the current entries update and ensure
the incremental merge reflects the single changed record equivalently.
Reconciles the Company Portal LocalState tail work with main's chunk-invariant Device Inventory tail framing (#511): - src-tauri/src/watcher/tail.rs: keep main's segment-based inventory pipeline (decode_tail_bytes UTF-8 carry, process_inventory_* bounded framing, flush_pending_text, finalize_pending_input) and re-apply the branch's Company Portal logical-record machinery on top (initial-record amendments, debounce flush, frame_company_portal_logical_records). The debounce timer is now gated to Company Portal owners so inventory framing stays chunk invariant; finalize dispatches by pending-state owner; truncation discards unpublished Company Portal continuations while preserving main's inventory finalize-on-truncation semantics. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src-tauri/src/watcher/tail.rs (1)
1174-1192: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
assign_physical_entry_identityduplicatesassign_framed_entry_identityexactly.Both methods run the same three statements: stamp
self.next_id, rebaseentry.line_numberonto the start line, then advanceself.next_linebyphysical_lines. The only difference is the doc comment and what the caller passes asphysical_lines.The numbering rule is what keeps "go to line" consistent between a tailed file and the same file opened. Two copies of that rule can drift independently. Keep one method and document both meanings of
physical_lineson it.♻️ Collapse to one numbering method
- fn assign_physical_entry_identity(&mut self, entries: &mut [LogEntry], physical_lines: u32) { - let batch_start = self.next_line; - for entry in entries { - entry.id = self.next_id; - entry.line_number = batch_start.saturating_add(entry.line_number.saturating_sub(1)); - self.next_id += 1; - } - self.next_line = batch_start.saturating_add(physical_lines); - }Call
assign_framed_entry_identityfrom the physical-line path instead.🤖 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/watcher/tail.rs` around lines 1174 - 1192, Remove the duplicate assign_physical_entry_identity method and reuse assign_framed_entry_identity from the physical-line path. Update the remaining method’s documentation to describe both meanings of physical_lines, preserving the existing ID assignment, line rebasing, and next_line advancement behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src-tauri/src/commands/file_ops.rs`:
- Around line 398-427: Update the aggregate_entry_lookup construction in the
surrounding folder-load flow to index only paths whose parser_selection passes
InitialLogicalRecord::supports_parser, avoiding allocation when no files are
seedable. Remove the fallible propagation from index_aggregate_entries and treat
duplicate coordinates as an unavailable seed for the affected file, while
preserving normal seeding for unique coordinates.
In `@src-tauri/src/watcher/tail.rs`:
- Around line 1495-1501: Update the poll interval selection near
company_portal_logical_framing so Company Portal tails use the slower cadence
when both pending_initial_logical_record and pending_logical_record are empty,
switching to LOGICAL_RECORD_DEBOUNCE only while either pending continuation
state exists; preserve the existing 500 ms interval for non-logical framing.
- Around line 1006-1019: The debounce flush path must not consume an
unterminated seeded fragment as a complete logical line. Update the handling
around pending_initial_logical_record and consume_initial_company_portal_lines
so fragments without a terminating newline remain buffered until continuation
arrives, or ensure the later remainder does not increment the physical-line
counter; preserve subsequent line_number and observed_through_line values.
- Around line 1293-1308: Update the overflow loop handling around pending_record
to remove the expect call and use Option::take_if for the length guard and
extraction. Preserve the existing split, completed_records, overflow_count, and
remainder processing while ensuring the loop only processes an oversized record
when one is present.
In `@src/hooks/use-file-watcher.test.tsx`:
- Around line 184-228: Add a test case in the useFileWatcher suite that sends a
valid parserSelection value of companyPortal through the tail listener on the
single-file path, then assert the parserSelection field is accepted and written
to the log store via setParserSelection. Keep the existing malformed-payload
assertions and state setup patterns unchanged.
In `@src/hooks/use-file-watcher.ts`:
- Around line 119-124: Update the invalid-payload branch in the tail-new-entries
listener to include the source file path from the raw event payload in the
console.error message. Preserve the existing early return and use the available
payload data before parseTailPayload discards it.
---
Outside diff comments:
In `@src-tauri/src/watcher/tail.rs`:
- Around line 1174-1192: Remove the duplicate assign_physical_entry_identity
method and reuse assign_framed_entry_identity from the physical-line path.
Update the remaining method’s documentation to describe both meanings of
physical_lines, preserving the existing ID assignment, line rebasing, and
next_line advancement behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9a7b080d-80e8-47e7-b37e-bdab923d20f2
⛔ Files ignored due to path filters (18)
crates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/code-tokens/Log_1.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/encoding-utf16le/Log_1.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/encoding-utf8-bom/Log_1.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/encoding-utf8-nobom/Log_1.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/invalid-timestamp/Log_1.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/malformed-structural-token/Log_1.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/multiline-continuation/Log_1.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/negative-generic-timestamped/Log_1.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/negative-unrelated-uwp/Log_1.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/redaction/Log_1.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/rotation/Log_1.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/rotation/Log_2.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/same-timestamp-distinct-activity/Log_1.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/severity-levels/Log_1.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v12-0-0/truncated-boundaries/Log_1.logis excluded by!**/*.logcrates/cmtraceopen-parser/tests/fixtures/intune/portal/windows/logs/v13-4-2/unknown-app-version/Log_1.logis excluded by!**/*.logsrc-tauri/tests/corpus/company_portal/clean/Log_1.logis excluded by!**/*.logsrc-tauri/tests/corpus/company_portal/negative/Log_1.logis excluded by!**/*.log
📒 Files selected for processing (31)
.gitattributes.github/workflows/cmtrace-ci.ymlcrates/cmtraceopen-parser/src/esp/mod.rscrates/cmtraceopen-parser/src/esp/redaction.rscrates/cmtraceopen-parser/src/intune/portal/windows/company_portal/logs/detect.rscrates/cmtraceopen-parser/src/intune/portal/windows/company_portal/logs/document.rscrates/cmtraceopen-parser/src/intune/portal/windows/company_portal/logs/entries.rscrates/cmtraceopen-parser/src/intune/portal/windows/company_portal/logs/framing.rscrates/cmtraceopen-parser/src/intune/portal/windows/company_portal/logs/grammar.rscrates/cmtraceopen-parser/src/intune/portal/windows/company_portal/logs/mod.rscrates/cmtraceopen-parser/src/intune/portal/windows/company_portal/logs/models.rscrates/cmtraceopen-parser/src/intune/portal/windows/company_portal/logs/redaction.rscrates/cmtraceopen-parser/src/models/log_entry.rscrates/cmtraceopen-parser/src/parser/detect.rscrates/cmtraceopen-parser/src/parser/mod.rscrates/cmtraceopen-parser/tests/company_portal_windows_logs.rsreferences/log-intune-reference.mdsrc-tauri/src/commands/bundle_ops.rssrc-tauri/src/commands/file_ops.rssrc-tauri/src/commands/parsing.rssrc-tauri/src/state/app_state.rssrc-tauri/src/watcher/tail.rssrc-tauri/tests/parser_supported_formats.rssrc/hooks/use-file-watcher.test.tsxsrc/hooks/use-file-watcher.tssrc/lib/column-config.tssrc/lib/tail-payload-validation.test.tssrc/lib/tail-payload-validation.tssrc/stores/log-store.test.tssrc/stores/log-store.tssrc/types/log.ts
Comment on lines
+398
to
+427
| { | ||
| let aggregate_entry_lookup = index_aggregate_entries(&aggregate_entries)?; | ||
|
|
||
| let mut open_files = state | ||
| .open_files | ||
| .lock() | ||
| .map_err(|e| crate::error::AppError::State(e.to_string()))?; | ||
| for ( | ||
| path_buf, | ||
| file_path, | ||
| parser_selection, | ||
| byte_offset, | ||
| file_total_lines, | ||
| final_entry_line_number, | ||
| ) in open_file_states | ||
| { | ||
| let initial_logical_record = if InitialLogicalRecord::supports_parser(&parser_selection) | ||
| { | ||
| final_entry_line_number | ||
| .and_then(|line_number| { | ||
| aggregate_entry_lookup | ||
| .get(&(file_path.as_str(), line_number)) | ||
| .copied() | ||
| }) | ||
| .and_then(|entry| { | ||
| InitialLogicalRecord::from_entry(entry, file_total_lines, &parser_selection) | ||
| }) | ||
| } else { | ||
| None | ||
| }; |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Build the lookup only for files that can be seeded.
Line 399 indexes every aggregate entry on every folder load. The map is queried at most once per file, and only when InitialLogicalRecord::supports_parser is true at line 414. For a folder with no Company Portal file, the whole map is pure allocation. The ? at line 399 also turns one duplicate coordinate into a failure of the entire folder load, which is a heavy penalty for a seeding optimization.
Restrict the index to the paths whose parser_selection passes supports_parser, and degrade a duplicate coordinate to "no seed for that file" instead of an error.
♻️ Scope the index to seedable files
- let aggregate_entry_lookup = index_aggregate_entries(&aggregate_entries)?;
+ let seedable_paths: std::collections::HashSet<&str> = open_file_states
+ .iter()
+ .filter(|(_, _, selection, _, _, _)| {
+ InitialLogicalRecord::supports_parser(selection)
+ })
+ .map(|(_, file_path, _, _, _, _)| file_path.as_str())
+ .collect();
+ let aggregate_entry_lookup =
+ index_aggregate_entries(&aggregate_entries, &seedable_paths);🤖 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/commands/file_ops.rs` around lines 398 - 427, Update the
aggregate_entry_lookup construction in the surrounding folder-load flow to index
only paths whose parser_selection passes InitialLogicalRecord::supports_parser,
avoiding allocation when no files are seedable. Remove the fallible propagation
from index_aggregate_entries and treat duplicate coordinates as an unavailable
seed for the affected file, while preserving normal seeding for unique
coordinates.
Comment on lines
+1006
to
+1019
| if self.pending_initial_logical_record.is_some() { | ||
| if !fragment.is_empty() { | ||
| let fragment_lines = [fragment.as_str()]; | ||
| if let Some(initial) = | ||
| self.consume_initial_company_portal_lines(&fragment_lines, Instant::now()) | ||
| { | ||
| let is_new_header = initial.remaining_start == Some(0); | ||
| batch.append(initial.batch); | ||
| if !is_new_header { | ||
| fragment.clear(); | ||
| fragment_selection = None; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check whether any test flushes a mid-line Company Portal fragment
# against a seeded initial record, and how next_continuation_line advances.
set -uo pipefail
echo '--- flush_pending_logical_record callers and seeded fragment handling ---'
rg -nP -C6 'flush_pending_logical_record|pending_initial_logical_record' src-tauri/src/watcher/tail.rs
echo '--- tests that write an unterminated fragment then flush ---'
rg -nP -C15 'write!\(file, "[^"]*"\)[^;]*;\s*$' src-tauri/src/watcher/tail.rs \
| rg -n -C15 'flush_pending_logical_record'
echo '--- next_continuation_line write sites ---'
ast-grep run --lang rust --pattern 'pending.next_continuation_line = $_' src-tauri/src/watcher/tail.rsRepository: adamgell/cmtraceopen
Length of output: 14008
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- read_new_entries framing and flush path ---'
sed -n '300,520p' src-tauri/src/watcher/tail.rs
sed -n '994,1065p' src-tauri/src/watcher/tail.rs
echo '--- continuation counter implementation ---'
sed -n '1200,1265p' src-tauri/src/watcher/tail.rs
echo '--- relevant test fixtures and assertions ---'
sed -n '2470,2680p' src-tauri/src/watcher/tail.rs
sed -n '2970,3070p' src-tauri/src/watcher/tail.rsRepository: adamgell/cmtraceopen
Length of output: 28017
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
# Read-only model of the relevant state transition. It compares a complete
# physical line with the same line split across a debounce flush.
class Model:
def __init__(self):
self.next_line = 1
self.pending_fragment = ""
self.pending_initial = True
self.amendments = []
def append_initial_continuation(self, text):
start = self.next_line
self.next_line += 1
self.amendments.append((start, self.next_line - 1, text))
def flush(self):
if self.pending_fragment:
self.append_initial_continuation(self.pending_fragment)
self.pending_fragment = ""
def read_complete_line(self, text):
self.append_initial_continuation(text)
def read_fragment(self, text):
self.pending_fragment += text
def finish(self):
self.flush()
def state(self):
return self.next_line, self.amendments
direct = Model()
direct.read_complete_line("continuation")
direct.read_complete_line("next header")
direct.finish()
split = Model()
split.read_fragment("contin")
split.flush()
split.read_complete_line("uation")
split.read_complete_line("next header")
split.finish()
print("direct:", direct.state())
print("split: ", split.state())
print("counter_shift:", split.next_line - direct.next_line)
PYRepository: adamgell/cmtraceopen
Length of output: 309
Do not consume an unterminated seeded fragment during debounce flush.
When a writer pauses mid-line, flush_pending_logical_record counts the fragment and clears it. The later remainder is counted as a second physical line, shifting subsequent line_number and observed_through_line values by one. Keep the fragment buffered until its newline arrives, or prevent the remainder from incrementing the counter.
🤖 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/watcher/tail.rs` around lines 1006 - 1019, The debounce flush
path must not consume an unterminated seeded fragment as a complete logical
line. Update the handling around pending_initial_logical_record and
consume_initial_company_portal_lines so fragments without a terminating newline
remain buffered until continuation arrives, or ensure the later remainder does
not increment the physical-line counter; preserve subsequent line_number and
observed_through_line values.
Comment on lines
+1293
to
+1308
| while pending_record | ||
| .as_ref() | ||
| .is_some_and(|record| record.len() > max_pending_bytes) | ||
| { | ||
| let mut record = pending_record | ||
| .take() | ||
| .expect("oversized Company Portal record must exist"); | ||
| let split_at = previous_char_boundary(&record, max_pending_bytes); | ||
| let remainder = record.split_off(split_at); | ||
| completed_records.push(FramedLogicalRecord { | ||
| physical_lines: u32::try_from(record.matches('\n').count()).unwrap_or(u32::MAX), | ||
| content: record, | ||
| }); | ||
| overflow_count = overflow_count.saturating_add(1); | ||
| pending_record = (!remainder.is_empty()).then_some(remainder); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Confirm the Rust toolchain level for Option::take_if availability.
set -uo pipefail
fd -H -t f 'rust-toolchain(\.toml)?$|\.tool-versions$' . --exec cat {}
rg -n 'rust-version|edition' src-tauri/Cargo.toml crates/cmtraceopen-parser/Cargo.toml
rg -n -C3 'toolchain|dtolnay/rust' .github/workflows/cmtrace-ci.ymlRepository: adamgell/cmtraceopen
Length of output: 1761
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- target code ---'
sed -n '1275,1315p' src-tauri/src/watcher/tail.rs
printf '%s\n' '--- toolchain declarations ---'
rg -n -C2 'rust-version|toolchain|edition' src-tauri/Cargo.toml crates/cmtraceopen-parser/Cargo.toml .github/workflows/cmtrace-ci.yml
printf '%s\n' '--- local compiler ---'
rustc --version 2>/dev/null || true
printf '%s\n' '--- take_if references ---'
rg -n 'take_if' . --glob '*.rs' --glob '*.toml' --glob '*.yml' --glob '*.yaml' || trueRepository: adamgell/cmtraceopen
Length of output: 4717
🌐 Web query:
Rust std::option::Option::take_if stabilization version Rust 1.88
💡 Result:
The method Option::take_if was stabilized in Rust 1.81.0, not Rust 1.88 [1]. It was stabilized via pull request #126089 [2], which was merged in June 2024 [2][3]. The Rust 1.81.0 release notes explicitly list Option::take_if among the stabilized APIs [1]. By the time of the Rust 1.88.0 release in June 2025, this method had already been stable for several versions [4][5][6].
Citations:
- 1: https://dev-doc.rust-lang.org/1.81.0/releases.html
- 2: Stabilize Option::take_if rust-lang/rust#126089
- 3: https://this-week-in-rust.org/blog/2024/06/12/this-week-in-rust-551/
- 4: https://doc.rust-lang.org/stable/releases.html
- 5: https://rust.googlesource.com/rust/+/refs/tags/1.88.0/RELEASES.md
- 6: https://blog.rust-lang.org/2025/06/26/Rust-1.88.0/
Remove the expect from the overflow loop.
This expect processes parsed log data and violates the backend rule. Replace the guard and take() pair with Option::take_if, which is available with the Rust 1.88 MSRV.
🤖 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/watcher/tail.rs` around lines 1293 - 1308, Update the overflow
loop handling around pending_record to remove the expect call and use
Option::take_if for the length guard and extraction. Preserve the existing
split, completed_records, overflow_count, and remainder processing while
ensuring the loop only processes an oversized record when one is present.
Source: Path instructions
Comment on lines
+1495
to
+1501
| let poll_interval = if company_portal_logical_framing(&tail_reader.parser_selection) { | ||
| // The continuation debounce only makes progress when the reader is | ||
| // polled, so a Company Portal tail polls on the debounce cadence. | ||
| LOGICAL_RECORD_DEBOUNCE | ||
| } else { | ||
| Duration::from_millis(500) | ||
| }; |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial
A 250 ms poll per Company Portal tail multiplies in aggregate-folder mode.
open_log_folder_aggregate starts one tail session per file, and each session runs its own thread and its own recv_timeout loop. read_new_entries opens the file and reads its metadata on every wakeup at line 312. For a Company Portal folder the wakeup rate is now 4 per second per file, so a folder with 30 logs performs about 120 open plus stat calls per second while nothing is being written.
The debounce only needs a wakeup when pending continuation state exists. Consider polling at the slower cadence when pending_initial_logical_record and pending_logical_record are both empty, and switching to the debounce cadence only while state is held.
🤖 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/watcher/tail.rs` around lines 1495 - 1501, Update the poll
interval selection near company_portal_logical_framing so Company Portal tails
use the slower cadence when both pending_initial_logical_record and
pending_logical_record are empty, switching to LOGICAL_RECORD_DEBOUNCE only
while either pending continuation state exists; preserve the existing 500 ms
interval for non-logical framing.
Comment on lines
+184
to
+228
| it("rejects malformed tail line and amendment coordinates at the event boundary", async () => { | ||
| const entry = { ...multilineEntry(), message: "[Sync] started" }; | ||
| useLogStore.setState({ | ||
| openFilePath: "/logs/Log_1.log", | ||
| sourceOpenMode: "single-file", | ||
| formatDetected: "Timestamped", | ||
| byteOffset: 512, | ||
| totalLines: 1, | ||
| entries: [entry], | ||
| }); | ||
| const errorSpy = vi.spyOn(console, "error").mockImplementation(() => undefined); | ||
|
|
||
| renderHook(() => useFileWatcher()); | ||
| await waitFor(() => expect(eventMocks.tailListener).not.toBeNull()); | ||
|
|
||
| act(() => { | ||
| eventMocks.tailListener?.({ | ||
| payload: { | ||
| ...emptyTailPayload(), | ||
| observedThroughLine: -1, | ||
| }, | ||
| }); | ||
| eventMocks.tailListener?.({ | ||
| payload: { | ||
| ...emptyTailPayload(), | ||
| amendments: [ | ||
| { | ||
| entryId: 0, | ||
| entryLineNumber: 1, | ||
| continuationStartLine: 0, | ||
| continuationEndLine: 2, | ||
| messageUtf16Start: 14, | ||
| messageSuffix: "\ninvalid", | ||
| errorCodeSpans: [], | ||
| }, | ||
| ], | ||
| }, | ||
| }); | ||
| }); | ||
|
|
||
| expect(errorSpy).toHaveBeenCalledTimes(2); | ||
| expect(useLogStore.getState().entries).toEqual([entry]); | ||
| expect(useLogStore.getState().totalLines).toBe(1); | ||
| errorSpy.mockRestore(); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add a case for the parserSelection branch.
The suite exercises entries, amendments, resets, parse errors, and observed lines, but no payload carries parserSelection. Lines 186-188 of src/hooks/use-file-watcher.ts call setParserSelection on the single-file path, and isParserSelection in src/lib/tail-payload-validation.ts gates that field against three hand-maintained allowlists. A payload with a valid parserSelection for companyPortal would prove both the validator entry and the store write in one case.
🤖 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/hooks/use-file-watcher.test.tsx` around lines 184 - 228, Add a test case
in the useFileWatcher suite that sends a valid parserSelection value of
companyPortal through the tail listener on the single-file path, then assert the
parserSelection field is accepted and written to the log store via
setParserSelection. Keep the existing malformed-payload assertions and state
setup patterns unchanged.
Comment on lines
+119
to
+124
| const unlisten = listen<unknown>("tail-new-entries", (event) => { | ||
| const payload = parseTailPayload(event.payload); | ||
| if (!payload) { | ||
| console.error("Ignored invalid tail payload from the backend"); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Log which file produced the rejected payload.
Line 122 emits a constant string. When validation fails, the batch is discarded and the tail for that file stops advancing with no way to identify the file or the failing field. The raw payload is available here even though parseTailPayload returns only null.
🩺 Attach the file path
const payload = parseTailPayload(event.payload);
if (!payload) {
- console.error("Ignored invalid tail payload from the backend");
+ const path =
+ typeof event.payload === "object" &&
+ event.payload !== null &&
+ "filePath" in event.payload
+ ? String((event.payload as { filePath: unknown }).filePath)
+ : "unknown file";
+ console.error(`Ignored invalid tail payload for ${path}`);
return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const unlisten = listen<unknown>("tail-new-entries", (event) => { | |
| const payload = parseTailPayload(event.payload); | |
| if (!payload) { | |
| console.error("Ignored invalid tail payload from the backend"); | |
| return; | |
| } | |
| const unlisten = listen<unknown>("tail-new-entries", (event) => { | |
| const payload = parseTailPayload(event.payload); | |
| if (!payload) { | |
| const path = | |
| typeof event.payload === "object" && | |
| event.payload !== null && | |
| "filePath" in event.payload | |
| ? String((event.payload as { filePath: unknown }).filePath) | |
| : "unknown file"; | |
| console.error(`Ignored invalid tail payload for ${path}`); | |
| return; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/hooks/use-file-watcher.ts` around lines 119 - 124, Update the
invalid-payload branch in the tail-new-entries listener to include the source
file path from the raw event payload in the console.error message. Preserve the
existing early return and use the available payload data before parseTailPayload
discards it.
This branch widens the ESP Windows CI job to run the full parser crate on Windows, and that new coverage exposed a latent bug: two fixture contract tests project filesystem paths with to_string_lossy and compare them against forward-slash manifest strings, so they fail on Windows with backslash separators. The management fixture contract's evidence projection now normalizes through the same normalize_manifest_relative_path helper the inventory/compliance contract already uses, clearing the four manifest-projection failures seen in CI. The updates fixture contract's corpus paths are normalized before feeding the FNV1a64 and SHA-256 corpus hashes, which would otherwise have been the next Windows failure once the first suite passed (cargo test stops at the first failing binary, so suites after the management contract have never yet run on Windows). Audited every other to_string_lossy/strip_prefix projection in the parser test corpus: the rest are single-component file_name calls, Path-to-Path joins, or already normalized (the software update point contract replaces MAIN_SEPARATOR itself). Verified: cargo test -p cmtraceopen-parser --test sccm_client_management_fixture_contract (29 passed) --test sccm_client_updates_fixture_contract (17 passed), cargo clippy -p cmtraceopen-parser --all-targets -- -D warnings clean. Windows validation rides the PR's own widened CI job. Refs #366 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The app_lib unit-test exe fails to load on this branch with STATUS_ENTRYPOINT_NOT_FOUND while the identically named exe loads on main, cold cache on both, same runner image and toolchain. This temporary step builds the exe with --no-run, dumps its import table with dumpbin, lists the DLLs beside it, and attempts a direct launch, so the unresolvable import is visible in the CI log. Reverted once the root cause is identified. Refs #366 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…s step" This reverts commit e485cac.
The widened ESP Windows job runs the full parser-crate suite and its clippy pass before the cmtrace-open steps. When all of that shares one target dir on a cold cache, the app_lib unit-test exe that the final cargo test step produces fails to load with STATUS_ENTRYPOINT_NOT_FOUND, even though its import table contains only standard system DLLs and the identically configured build on main's narrow job loads fine. A probe of plain main plus only the job widening reproduces the interaction, so the trigger is the widened job, not this branch's code. Giving the two parser steps their own CARGO_TARGET_DIR removes the artifact interaction entirely at the cost of rebuilding the parser crate's (pure-Rust, lean) dependency graph once. Refs #366 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This was referenced Aug 8, 2026
This reverts commit aceb2d1.
Root cause of the Windows CI loader failure on this branch: the aggregate_tail_seed_uses_the_frontend_visible_entry_id test built a tauri::test::mock_app() solely to obtain a tauri::State<AppState>. Instantiating the mock runtime statically anchors the windowing stack into the app_lib unit-test exe, adding roughly 6.6MB and imports of comctl32 v6's TaskDialogIndirect plus the menu/DWM surface. A bare test exe carries no comctl32-v6 manifest, so the Windows loader binds comctl32 5.x, cannot resolve TaskDialogIndirect, and kills the exe with STATUS_ENTRYPOINT_NOT_FOUND before any of its 590 tests run. Verified by diffing dumpbin import tables of this branch's exe (29,204,992 bytes, launch fails) against main's (22,564,352 bytes, launch succeeds): the delta is exactly the GUI import surface. The command body moves into open_log_folder_aggregate_impl taking a plain &AppState; the #[tauri::command] wrapper delegates to it, and the test drives the impl directly with AppState::default(). No mock app, no Manager import, identical assertions. Also reverts the CARGO_TARGET_DIR isolation attempt (aceb2d1): the shared-target-dir theory this fix disproves was wrong, and the isolation only cost a duplicate dependency build. Verified: cargo test --all-features --lib (590 passed, 0 failed) and cargo clippy --all-targets --all-features -- -D warnings, both from src-tauri. Refs #366, #523 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This was referenced Aug 8, 2026
adamgell
added a commit
that referenced
this pull request
Aug 13, 2026
…under Unreleased (#530) Catch up the Unreleased section with all ten commits merged since the last changelog update (54d539e / #513): - Microsoft Store app evidence lane (#358 / #518) - Reducer Framework v1 governance, ADRs, and charters (#519) - Windows Autopilot evidence parser outside ESP (#362 / #450) - Company Portal Windows LocalState logs (#366 / #460) - Bounded advanced SCCM server capture (#500), folded into the existing native SCCM diagnostics path bullet - Agent tooling / Clairvoyance staff org scaffolding (#516) - Dependency bumps (quick-xml, time, install-action) and the GitHub Sponsors funding link No version bump: package.json/Cargo.toml/tauri.conf.json remain at 1.5.1 with no new tag, so this stays purely an Unreleased catch-up. Claude-Session: https://claude.ai/code/session_01A8z5Ysfa5Afts6gVPmHQZj Co-authored-by: Claude <noreply@anthropic.com>
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Scope
Implements the first raw-format parser for Windows Company Portal
LocalStateLog_<n>.logfiles for #366 (part of #356). Restacked on currentmainvianon-destructive merge (no force-push).
It adds content-confirmed detection, logical-record framing, typed
LogEntry-compatible projection, version-scoped raw evidence documents,synthetic/sanitized fixtures, default redaction, and the required parser/UI
registration. The pure parser crate performs no filesystem collection or live
Windows access.
Evidence and safety boundaries
Log_<n>.logname only nominates acandidate; record structure must confirm it.
versions are experimental/low-confidence coverage, not validated facts.
logs, generic timestamped logs, and same-time distinct activities are all
covered by synthetic fixtures.
Available) coverage.with bounded amendments and physical-line provenance.
real-version capture, or a semantic root-cause engine.
Restack note (lane A)
Merged
origin/main(8064b5aa) intocodex/intune-366-review-fixes-r119to clear
CONFLICTINGstate. Sole content conflict was.gitattributes;resolved by retaining byte-sensitive fixture rules for parser corpora
(
-text -whitespace) including Company Portal and SCCM paths.Local verification after restack
Refs #366
Summary by CodeRabbit