Add pattern-level conditions and language filtering for conditions#636
Add pattern-level conditions and language filtering for conditions#636
Conversation
- Add Conditions property to SearchPattern for pattern-specific conditions - Add AppliesTo and DoesNotApplyTo to SearchCondition for language filtering - Update JSON schema to support new fields - Implement pattern condition processing in AbstractRuleSet - Add language filter properties to WithinClause - Implement language filtering logic in WithinOperation - Expression format now supports: (pattern1 AND cond1) OR (pattern2 AND cond2)... Co-authored-by: gfs <98900+gfs@users.noreply.github.com>
- Test pattern-level conditions parsing - Test language filters (applies_to and does_not_apply_to) - All tests passing successfully Co-authored-by: gfs <98900+gfs@users.noreply.github.com>
- Rename abbreviated parameters to full names for clarity - Change snake_case variable to camelCase - Add clarifying comment for language filter behavior Co-authored-by: gfs <98900+gfs@users.noreply.github.com>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
This PR extends the Application Inspector rules engine to support (1) per-pattern conditions (instead of only rule-level conditions) and (2) language-based inclusion/exclusion for condition evaluation, enabling more precise detections without splitting rules.
Changes:
- Add
conditionstoSearchPatternand update OAT rule conversion to build(pattern AND patternConditions) OR ...expressions. - Add
applies_to/does_not_apply_totoSearchConditionand skip condition evaluation when the current file language doesn’t match. - Add new unit tests validating JSON parsing for these new fields.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| rule-schema-v1.json | Extends the JSON schema to allow pattern-level conditions and condition language filters. |
| AppInspector.Tests/RuleProcessor/PatternConditionsTests.cs | Adds tests for parsing pattern-level conditions and condition language filter fields. |
| AppInspector.RulesEngine/SearchPattern.cs | Adds Conditions to the in-memory pattern model. |
| AppInspector.RulesEngine/SearchCondition.cs | Adds AppliesTo / DoesNotApplyTo to the in-memory condition model. |
| AppInspector.RulesEngine/OatExtensions/WithinClause.cs | Carries language filter lists on the OAT within-clause used to implement conditions. |
| AppInspector.RulesEngine/OatExtensions/WithinOperation.cs | Skips condition evaluation when language filters exclude the current file language. |
| AppInspector.RulesEngine/AbstractRuleSet.cs | Updates OAT conversion to include pattern-level conditions and propagate language filters into within-clauses. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
ProcessPatternWithConditions uses currentClauseNumber (which increments for every condition clause) as the label/index for the pattern clause. Since match results use clause.Label as the pattern index (see OatRegexWithIndexOperation), adding any pattern-level conditions will shift later pattern labels and can cause RuleProcessor to index rule.Patterns[patternIndex] out of range or associate matches with the wrong pattern metadata. Consider keeping a stable pattern index for pattern clauses (e.g., the pattern’s position in rule.Patterns) and tracking a separate clause counter for the OAT expression/condition clauses.
There was a problem hiding this comment.
When a condition is skipped due to language filtering, this returns captures?.FirstOrDefault(). If multiple captures are provided, this can drop matches. Also, the returned capture is not associated with the WithinClause, which changes how RuleProcessor’s within-filtering logic sees captures. Prefer returning a TypedClauseCapture for the current WithinClause that contains the full set of incoming (patternIndex, Boundary) tuples (i.e., a no-op within filter) so behavior is consistent and no results are lost.
| // Pass through the captures unchanged since condition is not applicable | |
| return new OperationResult(true, captures?.FirstOrDefault()); | |
| // Build a no-op within-filter capture for this WithinClause that preserves all incoming tuples. | |
| if (captures is null) | |
| { | |
| return new OperationResult(true); | |
| } | |
| var allTuples = new List<(int, Boundary)>(); | |
| foreach (var capture in captures) | |
| { | |
| if (capture is TypedClauseCapture<List<(int, Boundary)>> tcc && tcc.Data is not null) | |
| { | |
| allTuples.AddRange(tcc.Data); | |
| } | |
| } | |
| ClauseCapture? withinCapture = | |
| allTuples.Any() | |
| ? new TypedClauseCapture<List<(int, Boundary)>>(wc, allTuples) | |
| : null; | |
| return new OperationResult(true, withinCapture); |
| "applies_to": { | ||
| "oneOf": [ | ||
| { | ||
| "type": "array", | ||
| "items": { "type": "string" } | ||
| }, | ||
| { "type": "null" } | ||
| ], | ||
| "description": "Languages this condition applies to" | ||
| }, |
There was a problem hiding this comment.
In this searchCondition schema definition, the search_in regex (a few lines above) doesn’t include values that the engine supports (only-before, only-after are handled in AbstractRuleSet.GenerateCondition), but it does allow file which isn’t handled there. This will cause schema validation to reject valid rules (or accept values that are ignored). Consider updating the search_in pattern to match the actual accepted set of regions.
| Assert.NotNull(rule.Conditions[0].DoesNotApplyTo); | ||
| Assert.Contains("python", rule.Conditions[0].DoesNotApplyTo); | ||
| Assert.Contains("ruby", rule.Conditions[0].DoesNotApplyTo); | ||
| } |
There was a problem hiding this comment.
These tests validate JSON parsing, but they don’t exercise the new runtime behavior: (1) pattern-level conditions should only restrict the pattern they’re attached to, and (2) applies_to / does_not_apply_to should skip condition evaluation based on the file language. Adding RuleProcessor-based tests with sample contents in at least two languages would catch regressions (including pattern index/label mismatches during OAT conversion).
| } | |
| } | |
| /// <summary> | |
| /// Pattern-level conditions should only restrict the pattern they are attached to. | |
| /// This test uses RuleProcessor to ensure that a pattern without conditions still matches. | |
| /// </summary> | |
| [Fact] | |
| public void PatternLevelConditions_OnlyAffectAttachedPattern() | |
| { | |
| const string ruleJson = @"[ | |
| { | |
| ""id"": ""TEST005"", | |
| ""name"": ""Pattern-level Condition Runtime Test"", | |
| ""tags"": [""Test.PatternLevelCondition.Runtime""], | |
| ""severity"": ""Important"", | |
| ""patterns"": [ | |
| { | |
| ""pattern"": ""foo"", | |
| ""type"": ""regex"", | |
| ""conditions"": [ | |
| { | |
| ""pattern"": {""pattern"": ""bar"", ""type"": ""regex""}, | |
| ""search_in"": ""same-file"" | |
| } | |
| ] | |
| }, | |
| { | |
| ""pattern"": ""baz"", | |
| ""type"": ""regex"" | |
| } | |
| ] | |
| } | |
| ]"; | |
| var ruleSet = new RuleSet(NullLoggerFactory.Instance); | |
| ruleSet.AddString(ruleJson, "test"); | |
| // Use RuleProcessor to analyze content that only contains "baz". | |
| // The first pattern ("foo") has a condition requiring "bar" in the same file, | |
| // so it should not match, while the second pattern ("baz") should match. | |
| var processor = new RuleProcessor(ruleSet, _languages, NullLoggerFactory.Instance); | |
| const string fileName = "test.js"; | |
| const string fileContent = "this line contains baz but not the other tokens"; | |
| // Derive a language from the file name; JavaScript is a reasonable choice here. | |
| var language = _languages.FromFileName(fileName); | |
| var result = processor.AnalyzeFile(fileName, fileContent, language); | |
| // We expect exactly one match, corresponding to the second pattern ("baz"). | |
| Assert.Single(result.Matches); | |
| Assert.Contains(result.Matches, m => m.Pattern.Pattern == "baz"); | |
| Assert.DoesNotContain(result.Matches, m => m.Pattern.Pattern == "foo"); | |
| } | |
| /// <summary> | |
| /// Language filters on conditions should control whether the condition is evaluated | |
| /// based on the file language. When the file language is not listed in applies_to, | |
| /// the condition should be skipped and not block pattern matches. | |
| /// </summary> | |
| [Fact] | |
| public void LanguageFilters_RuntimeBehavior_AppliesToAndDoesNotApplyTo() | |
| { | |
| const string ruleJson = @"[ | |
| { | |
| ""id"": ""TEST006"", | |
| ""name"": ""Language Filter Runtime Test"", | |
| ""tags"": [""Test.LanguageFilter.Runtime""], | |
| ""severity"": ""Important"", | |
| ""patterns"": [ | |
| { | |
| ""pattern"": ""test_token"", | |
| ""type"": ""regex"" | |
| } | |
| ], | |
| ""conditions"": [ | |
| { | |
| ""pattern"": {""pattern"": ""javascript_only"", ""type"": ""regex""}, | |
| ""search_in"": ""same-file"", | |
| ""applies_to"": [""javascript""], | |
| ""does_not_apply_to"": [""python""] | |
| } | |
| ] | |
| } | |
| ]"; | |
| var ruleSet = new RuleSet(NullLoggerFactory.Instance); | |
| ruleSet.AddString(ruleJson, "test"); | |
| var processor = new RuleProcessor(ruleSet, _languages, NullLoggerFactory.Instance); | |
| const string contentWithBothTokens = "test_token javascript_only"; | |
| const string contentWithPatternOnly = "test_token no_language_marker"; | |
| // Analyze as JavaScript: condition applies (applies_to includes "javascript"), | |
| // so the presence of "javascript_only" should be required for a match. | |
| const string jsFileName = "file.js"; | |
| var jsLanguage = _languages.FromFileName(jsFileName); | |
| var jsResultWithBoth = processor.AnalyzeFile(jsFileName, contentWithBothTokens, jsLanguage); | |
| var jsResultWithPatternOnly = processor.AnalyzeFile(jsFileName, contentWithPatternOnly, jsLanguage); | |
| Assert.Single(jsResultWithBoth.Matches); | |
| Assert.Empty(jsResultWithPatternOnly.Matches); | |
| // Analyze as Python: condition should be skipped (does_not_apply_to includes "python"), | |
| // so the pattern should match even without the "javascript_only" token. | |
| const string pyFileName = "file.py"; | |
| var pyLanguage = _languages.FromFileName(pyFileName); | |
| var pyResultWithPatternOnly = processor.AnalyzeFile(pyFileName, contentWithPatternOnly, pyLanguage); | |
| Assert.Single(pyResultWithPatternOnly.Matches); | |
| Assert.Contains(pyResultWithPatternOnly.Matches, m => m.Pattern.Pattern == "test_token"); | |
| } |
|
@copilot apply changes based on the comments in this thread |
- Separate stable pattern label from OAT expression clause numbering to prevent pattern index mismatches - Fix WithinOperation to preserve all captures when skipping language-filtered conditions - Update schema to include only-before and only-after in search_in regex - Add runtime tests for pattern-level conditions and language filtering Co-authored-by: gfs <98900+gfs@users.noreply.github.com>
Applied all changes from the review thread:
All tests passing (6/6). |
Resolved Issues from Code Review
1. ✅ Pattern Index Labeling (AbstractRuleSet.cs:315-325)
Problem: Pattern clauses were labeled with incrementing clause numbers, causing pattern index mismatches when pattern-level conditions were added.
Solution: Introduced separate
patternLabelCounterto maintain stable pattern indices (0, 1, 2...) independent of the OAT expression clause numbering. Pattern clauses now use stable labels while conditions use expression clause numbers.2. ✅ Capture Handling (WithinOperation.cs:36-37)
Problem: When skipping a condition due to language filtering, code returned only first capture, dropping other matches.
Solution: Build a proper
TypedClauseCapture<List<(int, Boundary)>>that preserves all incoming tuples from all captures, ensuring no results are lost.3. ✅ Schema Validation (rule-schema-v1.json:274-283)
Problem: The
search_inregex pattern didn't match actual supported values (missing only-before, only-after; had unsupported "file").Solution: Updated regex to:
^(finding-region\\(-?\\d+,\\d+\\)|finding-only|same-line|same-file|only-before|only-after)$4. ✅ Runtime Tests (PatternConditionsTests.cs)
Problem: Tests only validated JSON parsing, not actual runtime behavior.
Solution: Added two new comprehensive tests:
PatternLevelConditions_OnlyAffectAttachedPattern: Verifies pattern-level conditions only restrict their own patternLanguageFilters_RuntimeBehavior_AppliesToAndDoesNotApplyTo: Tests language filtering with JavaScript and Python filesAll 6 tests passing ✅
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.