Skip to content

feat(collection): capture LocaleMetaData so exported channels are self-describing - #540

Merged
adamgell merged 6 commits into
mainfrom
feat/event-log-locale-metadata
Aug 12, 2026
Merged

feat(collection): capture LocaleMetaData so exported channels are self-describing#540
adamgell merged 6 commits into
mainfrom
feat/event-log-locale-metadata

Conversation

@adamgell

@adamgell adamgell commented Aug 9, 2026

Copy link
Copy Markdown
Owner

Closes part of #539 (Phase 2 groundwork).

Why now

wevtutil export-log emits only the binary records. Rendering an event description then requires the originating provider to be registered on whatever machine opens the file, which is never true for an analyst workstation and is impossible on macOS or Linux.

The timing argument is the important one: metadata cannot be recovered after the fact. A bundle collected today without LocaleMetaData is permanently unresolvable once that machine is rebuilt. Every bundle collected between now and whenever the reader ships is lossy for no reason. This lands ahead of the parser deliberately.

What changed

After each successful export the collector runs wevtutil archive-log, producing a LocaleMetaData\<name>_<lcid>.MTA sidecar carrying the provider's message strings. Each sidecar is recorded in the manifest as an event-log-metadata artifact with its own hash.

  • Archive failures degrade to an explicit coverage gap, not silence. The evtx stays collected.
  • Locale is left to the collecting machine's default so captured strings match what the operator saw on that endpoint. The emitted LCID is read from the produced filename rather than predicted.
  • -SkipLocaleMetadata opts out.

Cost measured on the lab host: +241 KB on a zipped seven-channel bundle (411,890 to 653,155 bytes).

Two pre-existing bugs fixed along the way

The collector could not run at all against the shipped profile, on either host. Both were found while trying to verify the change end to end, and both are fixed here because the feature could otherwise never execute:

  1. ConvertFrom-Json -Depth 20 at the profile load. -Depth does not exist on ConvertFrom-Json in Windows PowerShell 5.1 and fails parameter binding. That is the host the collector is deployed under.
  2. The optional-array check read values through Get-ObjectPropertyValue, which returns through the pipeline. That enumerates a single-element array such as arguments: ["/status"] down to a bare string, so the shipped profile was rejected as malformed.

The existing suite missed both because its fixture profile only used empty arrays, which enumerate to nothing and read as absent. New tests cover single-element, empty, and scalar cases, plus the shipped profile itself.

Worth noting on the second fix: my first attempt changed Get-ObjectPropertyValue to return with a unary comma. That is wrong. Callers collect it with @(...), which treats a comma-returned array as a single pipeline item and nests it one level deep. The tests caught it. The fix now reads the raw property at the one site that needs to distinguish array from scalar, matching what Assert-ProfileRequiredArray already does.

Verification

Run end to end on a Windows 11 x64 lab host (RING0IVY24-01, build 26200) under both hosts:

Host Result evtx MTA manifest records
Windows PowerShell 5.1 completed 83.6 s 9 9 9
pwsh 7.6 completed 82.9 s 9 9 9

Both previously failed at profile load. Artifact statuses collected=68 missing=5, the 5 being channels genuinely absent on that host and correctly recorded as coverage gaps.

Pester 14/14. Script parses clean, ASCII-only per the PS 5.1 rule.

Not included

Parsing .MTA is not in this PR. The container format is decoded and specced in #539 (magic MTAFile\0, three sections EVT/MSG/PUB); the inner section layouts still need reverse engineering. This PR only ensures the data is captured so that work has something to read.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features
    • Event-log evidence collection now archives channel locale metadata alongside exported logs.
    • Collection manifests record collected, missing, and failed metadata artifacts, including locale identifiers.
    • Added an option to skip locale metadata collection.
  • Bug Fixes
    • Improved reporting of missing, unreadable, and failed metadata artifacts.
    • Enhanced compatibility with Windows PowerShell 5.1.
  • Tests
    • Expanded coverage for metadata handling, profile validation, and optional data.

…f-describing

wevtutil export-log emits only the binary records. Rendering an event
description then requires the originating provider to be registered on
whatever machine opens the file, which is never true for an analyst
workstation and is impossible on macOS or Linux. Every bundle collected
without that metadata is permanently unresolvable once the source machine
is rebuilt, so this is captured now rather than after the reader exists.

After each successful export the collector runs wevtutil archive-log,
which writes a LocaleMetaData\<name>_<lcid>.MTA sidecar carrying the
provider's message strings. Each sidecar is recorded in the manifest as an
event-log-metadata artifact with its own hash. Archive failures degrade to
an explicit coverage gap rather than silence, leaving the evtx collected.
Locale is left to the collecting machine's default so the strings match
what the operator saw. -SkipLocaleMetadata opts out.

Measured on a Windows 11 x64 lab host: +241 KB on a zipped seven-channel
bundle (411,890 to 653,155 bytes).

This also fixes two pre-existing defects that stopped the collector
running at all against the shipped profile, on both hosts:

- ConvertFrom-Json was called with -Depth, which does not exist in
  Windows PowerShell 5.1 and fails parameter binding. That host is where
  the collector is deployed.
- The optional-array check read values through Get-ObjectPropertyValue,
  which returns through the pipeline. That enumerates a single-element
  array such as arguments: ["/status"] down to a bare string, so the
  shipped profile was rejected as malformed. The check now reads the raw
  property, matching what Assert-ProfileRequiredArray already does.

The existing suite missed both because its fixture profile only used
empty arrays, which enumerate to nothing and read as absent.

Verified end to end on Windows 11 x64 under both Windows PowerShell 5.1
and pwsh 7.6: 9 channels exported, 9 MTA sidecars, 9 manifest records.
Pester 14/14.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Copilot AI lite review requested due to automatic review settings August 9, 2026 14:52
@github-actions github-actions Bot added enhancement New feature or request feature New feature labels Aug 9, 2026
@coderabbitai

coderabbitai Bot commented Aug 9, 2026

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

The collector adds optional event-channel locale metadata archival. It derives sidecar paths, records collection gaps, updates the manifest, preserves optional-array validation, and removes a Windows PowerShell 5.1-incompatible JSON argument. Tests cover these behaviors.

Changes

Locale metadata collection

Layer / File(s) Summary
Metadata paths and collection controls
scripts/collection/Invoke-CmtraceEvidenceCollection.ps1, references/collection/Invoke-CmtraceEvidenceCollection.ps1, scripts/collection/tests/Invoke-CmtraceEvidenceCollection.Tests.ps1
The collector adds -SkipLocaleMetadata, derives LocaleMetaData paths, and extracts LCIDs from .MTA names. Tests cover path derivation and LCID handling.
Event-channel archival and manifest integration
scripts/collection/Invoke-CmtraceEvidenceCollection.ps1, references/collection/Invoke-CmtraceEvidenceCollection.ps1, scripts/collection/tests/Invoke-CmtraceEvidenceCollection.Tests.ps1
After successful event-log export, the collector uses wevtutil.exe al, records collected, missing, and failed artifacts, reports observed gaps, and updates the manifest unless metadata collection is skipped.
PowerShell validation and parsing compatibility
scripts/collection/Invoke-CmtraceEvidenceCollection.ps1, references/collection/Invoke-CmtraceEvidenceCollection.ps1, scripts/collection/tests/Invoke-CmtraceEvidenceCollection.Tests.ps1
Optional-array validation inspects raw properties. JSON parsing no longer uses the unsupported -Depth argument. Tests cover array shapes and Windows PowerShell 5.1 profile parsing.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Collector as Invoke-CmtraceEvidenceCollection
  participant EventUtil as wevtutil.exe
  participant Sidecar as LocaleMetaData sidecar
  participant Manifest as Bundle manifest
  Collector->>EventUtil: archive event channel with al
  EventUtil->>Sidecar: create MTA metadata sidecar
  Collector->>Sidecar: inspect archival output
  Collector->>Manifest: record metadata artifact or observed gap
Loading
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title follows Conventional Commits and clearly describes the collection change that captures locale metadata for exported event logs.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/event-log-locale-metadata

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@scripts/collection/Invoke-CmtraceEvidenceCollection.ps1`:
- Around line 792-794: Update the sidecar enumeration in the metadata-folder
branch around Get-ChildItem to use -ErrorAction Stop within try/catch instead of
silently continuing. On enumeration failure, add a failed artifact and record
the corresponding observed gap, while preserving normal missing-artifact
handling when enumeration succeeds with no matching files.
- Around line 803-806: Update the metadata artifact loop around
New-ArtifactRecord to extract the LCID from the final underscore-delimited
segment of $metadataFile.Name and include that value in $notes. Preserve the
existing locale, channel, and artifact-record fields while ensuring each
metadata artifact’s notes identify its parsed LCID.

In `@scripts/collection/tests/Invoke-CmtraceEvidenceCollection.Tests.ps1`:
- Around line 147-168: The existing Get-LocaleMetadataRelativePath tests cover
only path derivation; extend the suite with controlled tests for archive
failure, successful archival without sidecars, collected sidecar hashes,
observed gaps, and the -SkipLocaleMetadata opt-out. Use the relevant
collection/archive entry points and fixtures visible in the surrounding test
file, asserting each outcome against the manifest contract without relying on
real archive or filesystem 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: e1bfbb1e-d5de-47ad-9daf-8bad3084bb83

📥 Commits

Reviewing files that changed from the base of the PR and between f83f6e9 and 14ace9d.

📒 Files selected for processing (2)
  • scripts/collection/Invoke-CmtraceEvidenceCollection.ps1
  • scripts/collection/tests/Invoke-CmtraceEvidenceCollection.Tests.ps1

Comment thread scripts/collection/Invoke-CmtraceEvidenceCollection.ps1 Outdated
Comment thread scripts/collection/Invoke-CmtraceEvidenceCollection.ps1
Comment thread scripts/collection/tests/Invoke-CmtraceEvidenceCollection.Tests.ps1

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR extends the PowerShell evidence collector to capture Windows Event Log provider locale metadata (LocaleMetaData\*.MTA) alongside exported .evtx files, making event-log artifacts self-describing for offline analysis (including on macOS/Linux). It also fixes two compatibility/profile-shape bugs discovered during end-to-end verification and adds targeted Pester coverage.

Changes:

  • After each successful wevtutil epl export, run wevtutil archive-log and record produced .MTA sidecars as event-log-metadata artifacts (opt-out via -SkipLocaleMetadata).
  • Fix profile loading/validation issues: remove unsupported ConvertFrom-Json -Depth usage (PS 5.1) and correct optional-array validation to not mis-handle single-element arrays.
  • Add Pester tests covering optional-array validation, PS 5.1 host compatibility, shipped profile acceptance, and locale metadata path derivation.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
scripts/collection/Invoke-CmtraceEvidenceCollection.ps1 Adds locale metadata capture via wevtutil archive-log, adds -SkipLocaleMetadata, and fixes profile parsing + optional-array validation behavior.
scripts/collection/tests/Invoke-CmtraceEvidenceCollection.Tests.ps1 Adds regression tests for the profile-shape fixes and for locale metadata path handling.
Suppressed comments (1)

scripts/collection/Invoke-CmtraceEvidenceCollection.ps1:800

  • In the archive-log "reported success but produced no .MTA" path, the metadata artifact record again uses RelativePath = $metadataRelativeFolder (a directory). This can cause existsOnDisk to become true if the folder exists, and multiple missing metadata artifacts will share the same relativePath. Record a deterministic placeholder .MTA filename instead.
    if ($metadataFiles.Count -eq 0) {
        $notes = 'wevtutil.exe al reported success but produced no .MTA sidecar. Event descriptions will not resolve away from this machine.'
        $records.Add((New-ArtifactRecord -Category 'event-log-metadata' -Family $Family -RelativePath $metadataRelativeFolder -OriginPath $Channel -Status 'missing' -ParseHints @('mta') -Notes $notes))
        Add-ObservedGap -ObservedGaps $ObservedGaps -Status 'missing' -Origin $Channel -Reason $notes
        return $records

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.


if ($exitCode -ne 0) {
$notes = 'wevtutil.exe al failed with exit code {0}. Event descriptions will not resolve away from this machine.' -f $exitCode
$records.Add((New-ArtifactRecord -Category 'event-log-metadata' -Family $Family -RelativePath $metadataRelativeFolder -OriginPath $Channel -Status 'failed' -ParseHints @('mta') -Notes $notes))
crates/cmtraceopen-parser asserts scripts/collection and references/collection
hold byte-identical collector scripts. The previous commit changed only the
staged copy, so cross_profile_collectors_preserve_command_parse_hints failed
with "collection scripts drifted".

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 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 `@references/collection/Invoke-CmtraceEvidenceCollection.ps1`:
- Around line 803-806: Update the metadata record construction in the foreach
loop over $metadataFiles to parse the LCID suffix from $metadataFile.Name and
include the generated LCID in $notes (or the artifact manifest field) passed to
New-ArtifactRecord. Preserve the existing channel and metadata details while
ensuring every event-log-metadata record contains the required LCID.
- Around line 781-788: Wrap the `wevtutil.exe al $EvtxPath` invocation in the
surrounding collection function’s local try/catch so terminating errors are
handled without aborting. In the catch, add a failed `event-log-metadata` record
and matching `Add-ObservedGap` entry using the error details, then return
`$records`; preserve the existing `$LASTEXITCODE` handling for non-terminating
failures. Add a Pester test covering the terminating invocation-error path.
🪄 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: 3a908372-ce41-4afa-a839-9eeb0cdf5fe8

📥 Commits

Reviewing files that changed from the base of the PR and between 14ace9d and d3aad70.

📒 Files selected for processing (1)
  • references/collection/Invoke-CmtraceEvidenceCollection.ps1

Comment thread references/collection/Invoke-CmtraceEvidenceCollection.ps1 Outdated
Comment thread references/collection/Invoke-CmtraceEvidenceCollection.ps1
Four findings from the PR review, all valid.

An enumeration fault was reported as an absence. Get-ChildItem over the
LocaleMetaData folder used -ErrorAction SilentlyContinue, so an access or I/O
error produced an empty result and a 'missing' artifact, claiming the sidecar
was never created when it may simply be unreadable. It now runs with
-ErrorAction Stop inside try/catch and records a 'failed' artifact and observed
gap.

Unresolved outcomes pointed at a directory. The failed and missing records used
the LocaleMetaData folder as relativePath. Bundle inspection treats that field
as a file and tests it on disk, so those records read as present-on-disk
whenever the folder existed, and every channel's failure shared one path. They
now use a deterministic file-shaped <log>_unknown-lcid.MTA placeholder, unique
per channel.

The LCID was documented but never recorded. The function contract said the
emitted locale identifier is captured in the artifact notes; nothing parsed it.
Get-LocaleMetadataLcid reads it back from the sidecar name, taking the final
underscore-delimited segment so exported log names containing underscores still
resolve, and returning 'unknown' rather than guessing when the suffix is absent
or non-numeric.

Test coverage stopped at path derivation. Added cases for LCID extraction
including the underscore and non-numeric edges, the file-shaped unresolved path,
the enumeration-fault contract, and the -SkipLocaleMetadata opt-out.

Verified end to end on Windows 11 x64 under Windows PowerShell 5.1: the default
run produced 9 metadata records with notes reading "Locale metadata (LCID 1033)"
and correct hashes, and -SkipLocaleMetadata produced 0 records and 0 sidecars.
Pester 22/22, reference copy resynced so the parser drift test passes, script
parses clean and stays ASCII-only.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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 `@scripts/collection/Invoke-CmtraceEvidenceCollection.ps1`:
- Around line 828-839: Update
scripts/collection/Invoke-CmtraceEvidenceCollection.ps1 lines 828-839 and
references/collection/Invoke-CmtraceEvidenceCollection.ps1 lines 828-839 so the
existing try/catch around metadata enumeration also encloses Test-Path, with
Test-Path using -ErrorAction Stop; preserve the current failed-artifact and
observed-gap handling in the catch block for access or I/O errors.
🪄 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: 7b01c543-4451-4e51-9e2f-1cc946a8be7f

📥 Commits

Reviewing files that changed from the base of the PR and between d3aad70 and 209b595.

📒 Files selected for processing (3)
  • references/collection/Invoke-CmtraceEvidenceCollection.ps1
  • scripts/collection/Invoke-CmtraceEvidenceCollection.ps1
  • scripts/collection/tests/Invoke-CmtraceEvidenceCollection.Tests.ps1

Comment thread scripts/collection/Invoke-CmtraceEvidenceCollection.ps1 Outdated
Follow-up to the enumeration fix: Get-ChildItem was moved into a try, but the
Test-Path guarding it was left outside. This script sets
$ErrorActionPreference to 'Stop', so a provider or I/O fault while probing the
LocaleMetaData folder would abort the entire collection run rather than
recording one failed artifact for that channel, which is the opposite of the
coverage discipline the surrounding code follows.

Test-Path now runs inside the same try with -ErrorAction Stop, and the existing
failed-artifact path handles it. Reference copy resynced.

Pester 23/23, script parses clean and stays ASCII-only, collector drift test
passes.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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 `@scripts/collection/Invoke-CmtraceEvidenceCollection.ps1`:
- Line 1: Remove the U+FEFF BOM before [CmdletBinding()] in
scripts/collection/Invoke-CmtraceEvidenceCollection.ps1 at lines 1-1 and
references/collection/Invoke-CmtraceEvidenceCollection.ps1 at lines 1-1; save
both collector copies as ASCII or UTF-8 without BOM.
🪄 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: 29146cf7-358e-4066-afa5-be20ae09b666

📥 Commits

Reviewing files that changed from the base of the PR and between 209b595 and bf0b2de.

📒 Files selected for processing (3)
  • references/collection/Invoke-CmtraceEvidenceCollection.ps1
  • scripts/collection/Invoke-CmtraceEvidenceCollection.ps1
  • scripts/collection/tests/Invoke-CmtraceEvidenceCollection.Tests.ps1

Comment thread scripts/collection/Invoke-CmtraceEvidenceCollection.ps1 Outdated
The files opened with EF BB BF before [CmdletBinding()]. Nothing in either is
non-ASCII, so the BOM bought nothing and cost the one thing it can: any path
that reads the script as bytes rather than through PowerShell's own encoding
detection sees three junk characters ahead of the first token. Piping the file
into a remote shell is exactly such a path, and it is how this collector gets
deployed.

Verified on the real thing rather than reasoned about: PowerShell 5.1.26100
parses the stripped file to 7,303 tokens with no errors and builds a
scriptblock from all 55,124 characters.

Both copies stay byte-identical, which the parser crate asserts.

Also checked the rest of the class: neither copy contains any other non-ASCII
byte, so this was the only instance.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@adamgell

adamgell commented Aug 9, 2026

Copy link
Copy Markdown
Owner Author

Both open findings addressed.

BOM (new). Real: both copies opened with EF BB BF before [CmdletBinding()]. Nothing in either file is non-ASCII, so the BOM bought nothing and cost the one thing it can — any path reading the script as bytes rather than through PowerShell's own encoding detection sees three junk characters ahead of the first token. Piping the file into a remote shell is exactly such a path, and it is how this collector gets deployed. Fixed in b0af5922.

Verified on the real thing rather than reasoned about, on PowerShell 5.1.26100:

BOM present    : False
First 3 bytes  : 5B 43 6D
PARSE OK       : 7303 tokens
SCRIPTBLOCK OK : 55124 chars

Checked the rest of the class too: neither copy holds any other non-ASCII byte, so that was the only instance. Both copies stay byte-identical, which the parser crate asserts.

Test-Path inside the try (18:13Z). Already fixed on this branch. Test-Path is inside the try with -ErrorAction Stop, with a comment recording why, and the catch still produces the failed artifact and the observed gap:

try {
    if (Test-Path -LiteralPath $metadataFolder -ErrorAction Stop) {
        $metadataFiles = @(Get-ChildItem -LiteralPath $metadataFolder -Filter ... -ErrorAction Stop)
    }
}
catch {
    # An access or I/O fault here is a failure, not an absence.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
scripts/collection/Invoke-CmtraceEvidenceCollection.ps1 (1)

817-824: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Catch terminating wevtutil.exe al invocation errors in both collector copies.

If the invocation raises, the outer collection try exits before Export-EventChannelLocaleMetadata returns its failed metadata artifact and observed gap. Wrap the invocation and exit-code capture in a local try/catch. Preserve the existing nonzero exit-code branch in both files.

  • scripts/collection/Invoke-CmtraceEvidenceCollection.ps1#L817-L824
  • references/collection/Invoke-CmtraceEvidenceCollection.ps1#L817-L824
🤖 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 `@scripts/collection/Invoke-CmtraceEvidenceCollection.ps1` around lines 817 -
824, Wrap the wevtutil.exe invocation and $LASTEXITCODE capture in
Export-EventChannelLocaleMetadata with a local try/catch so terminating errors
produce the existing failed metadata artifact and observed gap instead of
escaping the outer collection try. Preserve the current nonzero exit-code
branch. Apply the same change in
scripts/collection/Invoke-CmtraceEvidenceCollection.ps1 lines 817-824 and
references/collection/Invoke-CmtraceEvidenceCollection.ps1 lines 817-824.

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.

Outside diff comments:
In `@scripts/collection/Invoke-CmtraceEvidenceCollection.ps1`:
- Around line 817-824: Wrap the wevtutil.exe invocation and $LASTEXITCODE
capture in Export-EventChannelLocaleMetadata with a local try/catch so
terminating errors produce the existing failed metadata artifact and observed
gap instead of escaping the outer collection try. Preserve the current nonzero
exit-code branch. Apply the same change in
scripts/collection/Invoke-CmtraceEvidenceCollection.ps1 lines 817-824 and
references/collection/Invoke-CmtraceEvidenceCollection.ps1 lines 817-824.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: bd69f8f7-4faf-4a44-a7d4-bb66c523491d

📥 Commits

Reviewing files that changed from the base of the PR and between bf0b2de and b0af592.

📒 Files selected for processing (2)
  • references/collection/Invoke-CmtraceEvidenceCollection.ps1
  • scripts/collection/Invoke-CmtraceEvidenceCollection.ps1

@adamgell

Copy link
Copy Markdown
Owner Author

@coderabbitai full review

Both open findings are addressed (BOM stripped in b0af592; the Test-Path case was already inside the try with -ErrorAction Stop). Please re-review the current head.

@coderabbitai

coderabbitai Bot commented Aug 10, 2026

Copy link
Copy Markdown

@adamgell Full review requested for the current head. I will verify the BOM removal and the Test-Path -ErrorAction Stop failure path during the review.

✅ Action performed

Full review finished.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@scripts/collection/Invoke-CmtraceEvidenceCollection.ps1`:
- Around line 817-825: Protect the wevtutil.exe al invocation and $LASTEXITCODE
read in both scripts/collection/Invoke-CmtraceEvidenceCollection.ps1:817-825 and
references/collection/Invoke-CmtraceEvidenceCollection.ps1:817-825 with
identical try/catch handling. In each catch, add a failed event-log-metadata
record using $unresolvedRelativePath, call Add-ObservedGap with status failed
and the error reason, then return $records; preserve the existing nonzero
exit-code handling and byte-identical content between both copies.

In `@scripts/collection/tests/Invoke-CmtraceEvidenceCollection.Tests.ps1`:
- Around line 191-231: Replace the five source-text regex tests in “Locale
metadata artifact contract” with behavior-based tests that load and invoke
Export-EventChannelLocaleMetadata, New-ArtifactRecord, and Add-ObservedGap
through the existing AST loader. Stub wevtutil.exe and the sidecar folder in a
temporary directory, then assert returned records’ status, relativePath, notes
containing the parsed LCID, hashes.sha256, and corresponding $ObservedGaps
entries; retain only the SkipLocaleMetadata declaration text assertion if
end-to-end collector invocation is impractical.
- Around line 109-126: Rename the local `$profile` variable to `$testProfile` in
the affected tests using `New-TestCollectorProfile`, including all assignments
and subsequent `Assert-CollectorProfileShape` references, to avoid shadowing
PowerShell’s automatic variable.
🪄 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: 5e910a5f-4c3d-4b54-a81c-11da9384ef52

📥 Commits

Reviewing files that changed from the base of the PR and between f83f6e9 and b0af592.

📒 Files selected for processing (3)
  • references/collection/Invoke-CmtraceEvidenceCollection.ps1
  • scripts/collection/Invoke-CmtraceEvidenceCollection.ps1
  • scripts/collection/tests/Invoke-CmtraceEvidenceCollection.Tests.ps1

Comment thread scripts/collection/Invoke-CmtraceEvidenceCollection.ps1 Outdated
Comment thread scripts/collection/tests/Invoke-CmtraceEvidenceCollection.Tests.ps1 Outdated
Comment thread scripts/collection/tests/Invoke-CmtraceEvidenceCollection.Tests.ps1 Outdated
…t source text

Three review findings, all real.

The wevtutil.exe call was unguarded. $ErrorActionPreference is 'Stop', so the
command being absent from PATH or failing to launch took down the entire
collection rather than recording one failed channel. That is the same class as
the Test-Path case fixed earlier in this branch; that fix closed one instance
and left the other open. A missing wevtutil.exe is not far-fetched on a locked
down host, and losing every other artifact in the bundle over one sidecar is
the worst available outcome.

$profile in the test suite shadows a PowerShell automatic variable. Renamed to
$testProfile. Checked the rest of the class across both scripts and the tests:
no other automatic variable is shadowed anywhere.

The locale metadata tests asserted the collector's source text matched five
regexes. That passes just as happily when the behaviour is wrong and breaks on
a harmless reformat. They now load Export-EventChannelLocaleMetadata through
the suite's existing AST loader and invoke it against a stubbed wevtutil.exe
and a temporary sidecar folder, asserting the returned records and the observed
gaps. Function definitions win over external commands in PowerShell's
resolution order, so the stub is reached without changing the collector, and
the tests run off Windows.

Verified the tests can actually fail: removing the wevtutil guard fails one,
and reporting an enumeration fault as 'missing' rather than 'failed' fails
another. 26 pass on Pester 6, and PowerShell 5.1.26100 parses the collector to
7,375 tokens with no errors.

Only the SkipLocaleMetadata assertion stays text-based, because the switch is a
parameter on the script itself and there is no unit to invoke.

Both copies stay byte-identical and ASCII with no BOM.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@adamgell

Copy link
Copy Markdown
Owner Author

All three addressed in 096c0577.

wevtutil.exe unguarded (real). $ErrorActionPreference is Stop, so the command being absent from PATH or failing to launch took down the entire collection instead of recording one failed channel. This is the same class as the Test-Path case fixed earlier on this branch — that fix closed one instance and left the sibling open. Losing every other artifact in a bundle over one sidecar is the worst available outcome, so it now records a failed event-log-metadata record plus the observed gap and returns.

$profile shadowing (real). Renamed to $testProfile. Audited the class rather than the instance: no other PowerShell automatic variable is shadowed anywhere in either collector copy or the test suite.

Source-text tests (real, and the most useful of the three). Five tests asserted the collector's text matched regexes, which passes just as happily when the behaviour is wrong and breaks on a harmless reformat. They now load Export-EventChannelLocaleMetadata through the suite's existing AST loader and invoke it against a stubbed wevtutil.exe and a temp sidecar folder, asserting returned record status, relativePath, notes carrying the parsed LCID, hashes.sha256, and the matching $ObservedGaps entries. Function definitions win over external commands in PowerShell's resolution order, so the stub is reached without touching the collector — which also means these run off Windows.

I checked the new tests can actually fail, rather than trusting a green run:

mutation result
remove the wevtutil guard 1 failed
report an enumeration fault as missing instead of failed 1 failed
unmutated 26 passed

Only the SkipLocaleMetadata assertion stays text-based: it is a parameter on the script itself, so there is no unit to invoke.

Verified on the real runtime too — PowerShell 5.1.26100 parses the collector to 7,375 tokens with no errors and builds a scriptblock from all 54,738 characters. Both copies stay byte-identical, ASCII, no BOM.

@adamgell

Copy link
Copy Markdown
Owner Author

Charter review — PR #540

Reviewed head: 096c05778404ae0010065c78f29b724b6c0aaeaf

Findings (most severe first)

No blocking findings. The implementation keeps the two collector copies byte-identical, records collected/missing/failed LocaleMetaData outcomes as coverage states, records LCIDs from the emitted filename, hashes collected sidecars, and uses a deterministic file-shaped placeholder for unresolved outcomes. The opt-out path skips metadata capture without changing the event-log artifact.

Named gate states

  • CI checks: PASS. Live PR checks at the reviewed head are successful: Rust check/test, Rust MSRV (Ubuntu and Windows), TypeScript, E2E, ESP Diagnostics (Windows), CodeQL analyses, and macOS/Windows/Linux builds. git diff --check is clean.
  • CodeRabbit review state at head (approved_at_head): PASS. The live CodeRabbit review is APPROVED on 096c0577; the CodeRabbit status check is also pass. Earlier actionable findings were verified against the final code and addressed.
  • Hermes charter review: POSTED, no open blocking findings. Focused verification passed: PowerShell AST parse (7,375 tokens) and Pester 6.0.0: 26 passed, 0 failed, 0 skipped.
  • Contract-layer conformance: PASS / NOT APPLICABLE as a reducer contract gate. This PR changes the Windows evidence-collection script and tests only; it does not touch a reducer, parser crate, identity/correlation, chronology, terminal precedence, or cross-lane semantic contract. Applicable evidence-contract checks pass: failures and absences remain explicit manifest/gap states, rather than being reported as collected evidence.

Review feedback rejected

None. No review feedback was rejected as invalid. CodeRabbit findings concerning LCID notes, invocation/enumeration failure handling, the BOM, and behavior-based tests were confirmed valid and are addressed. Copilot's suppressed concern about directory-shaped unresolved paths was also valid and is addressed by *_unknown-lcid.MTA; it was not rejected.

Gate state

Merge gate: READY on the reviewed head, subject to Adam's merge action. Hermes did not approve or merge the PR.

This review covered the origin/main...origin/feat/event-log-locale-metadata diff, both collector copies, the Pester suite, live PR checks/reviews, and the applicable collection/evidence contracts; it did not cover unrelated repository code, live Windows endpoint collection, or production tenant/device data.

@adamgell
adamgell merged commit 6ad7d0e into main Aug 12, 2026
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request feature New feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants