test(eventviewerx): ✅ enforce subject/target helper usage

[PrzemyslawKlys]

No description provided.

[claude]

Claude finished @PrzemyslawKlys's task —— View job

---

Code Review Analysis

Todo List:

• Check for repository-specific guidelines (CLAUDE.md)
• Read and analyze the modified test file
• Review code quality and best practices
• Check for potential bugs or issues
• Evaluate performance considerations
• Assess security concerns
• Analyze test coverage improvements
• Provide comprehensive feedback

Code Review Feedback

Overall Assessment ✅

This PR adds an excellent architectural guard test that enforces the use of helper methods for Subject/Target account field pairs. The implementation is well-thought-out and follows established patterns in the codebase.

🟢 Code Quality & Best Practices

Excellent implementation with strong adherence to best practices:

• Consistent Pattern: The new test RuleSources_DoNotUseDirectSubjectTargetKnownFieldPairCalls() (lines 82-96) follows the exact same pattern as existing guard tests
• Clear Intent: Method name clearly communicates its purpose - preventing direct Subject/Target field pair calls
• Descriptive Error Messages: Provides clear guidance on what to use instead (GetSubjectAccountOrEmpty()/GetTargetAccountOrEmpty())
• Proper Regex Pattern: Uses well-crafted regex to detect both SubjectUserName/SubjectDomainName and TargetUserName/TargetDomainName patterns
• Integration: Leverages existing FindRuleSourceOffenders() infrastructure effectively

🟢 Performance Considerations

Well-optimized implementation:

• Compiled Regex: Uses RegexOptions.Compiled for better performance during repeated test runs
• Efficient Pattern: Regex pattern is specific enough to avoid false positives while catching the target scenarios
• Reuses Infrastructure: Leverages existing file scanning logic rather than duplicating it

🟢 Security & Maintainability

No security concerns identified:

• Static Analysis: This is purely a static analysis test that scans source code - no runtime security implications
• Defensive Coding: Enforces better coding practices by preventing direct field access patterns
• Comment Skipping: Properly ignores commented code (lines 111-116), preventing false positives

🟢 Test Coverage Enhancement

Significantly improves architectural enforcement:

• API Design Enforcement: Ensures developers use the intended helper methods rather than low-level field access
• Consistency: Prevents inconsistent data access patterns across the rule codebase
• Maintainability: Makes future refactoring easier by ensuring consistent API usage

💡 Minor Suggestions for Improvement

1. Regex Pattern Enhancement (line 85-87): Consider using named groups for better maintainability:

   @"\bGetValueFromDataDictionary\s*\(\s*KnownEventField\.(?:Subject|Target)UserName\s*,\s*KnownEventField\.(?:Subject|Target)DomainName"

2. Future Extensibility: The pattern could potentially be made more generic to catch other paired field access patterns, but the current specific implementation is probably better for clarity.

🔍 Technical Analysis

Regex Pattern Breakdown:

• \bGetValueFromDataDictionary\s*\( - Matches the method call
• \s*KnownEventField\.SubjectUserName\s*,\s*KnownEventField\.SubjectDomainName - Matches first parameter pair
• |\b... - OR operator for the second pattern
• Properly handles whitespace variations

Integration Quality:
The test integrates seamlessly with the existing test infrastructure, reusing the FindRuleSourceOffenders() method and following the same assertion pattern.

✅ Final Verdict

This is an exemplary addition to the test suite that:

• Enforces good architectural practices
• Maintains code consistency
• Provides clear guidance to developers
• Follows established testing patterns perfectly

The implementation demonstrates thoughtful API design enforcement and will help maintain code quality as the project evolves.

---

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 45ffa230fa

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Match subject/target pair calls across line breaks

This guard only matches GetValueFromDataDictionary(KnownEventField..., KnownEventField...) when both enum arguments are on the same physical line, but FindRuleSourceOffenders checks each line independently, so a multiline invocation (a common style in this repo) will bypass the test entirely. That means the new test can report green while forbidden direct subject/target pair access still exists, weakening the policy it is meant to enforce.

Useful? React with 👍 / 👎.

[Copilot]

Pull request overview

Adds a new test guard in the EventViewerX test suite to enforce that rule sources use the dedicated subject/target account helpers instead of manually combining Subject/Target known-field pairs.

Changes:

• Introduces a new xUnit [Fact] that scans rule source files for direct GetValueFromDataDictionary(KnownEventField.Subject*/Target*) pair usage.
• Provides a clearer failure message directing contributors to GetSubjectAccountOrEmpty()/GetTargetAccountOrEmpty().

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The regex only matches GetValueFromDataDictionary(KnownEventField.SubjectUserName, KnownEventField.SubjectDomainName (and the Target equivalent) when both arguments are on the same line and in that specific order. Since FindRuleSourceOffenders tests one line at a time, a forbidden call split across multiple lines (or with the two KnownEventField arguments swapped + reverseOrder adjusted) would not be detected. Consider expanding the pattern to match both argument orders and/or changing the offender scan to run the regex over the full file text so multi-line invocations are caught.

Suggested change

	@"\bGetValueFromDataDictionary\s*\(\s*KnownEventField\.SubjectUserName\s*,\s*KnownEventField\.SubjectDomainName|\bGetValueFromDataDictionary\s*\(\s*KnownEventField\.TargetUserName\s*,\s*KnownEventField\.TargetDomainName",
	@"\bGetValueFromDataDictionary\s*\(\s*KnownEventField\.(?:SubjectUserName\s*,\s*KnownEventField\.SubjectDomainName|SubjectDomainName\s*,\s*KnownEventField\.SubjectUserName|TargetUserName\s*,\s*KnownEventField\.TargetDomainName|TargetDomainName\s*,\s*KnownEventField\.TargetUserName)",