Skip to content

Commit 0342edf

Browse files
Copilotdata-douser
Copilot
and
data-douser
authored
[NEW PRIMITIVE] sarif_diff_by_commits — SARIF-to-git-diff correlation tool (#236)
* Initial plan * feat: add sarif_diff_by_commits tool for SARIF-to-git-diff correlation Implements a new MCP tool that accepts a SARIF file path and git ref range, partitions SARIF results into "new" vs "pre-existing" based on file-level or line-level overlap with the git diff, and returns structured output for triage workflows. - Add diffSarifByCommits() pure utility in sarif-utils.ts with types - Register sarif_diff_by_commits tool in sarif-tools.ts - Add parseGitDiffOutput() helper for unified diff parsing - Add 14 unit tests for diffSarifByCommits() utility - Add 5 unit tests for sarif_diff_by_commits tool handler - Update server-tools.md documentation - Update tool registration count from 7 to 8 Closes #209 Agent-Logs-Url: https://github.com/advanced-security/codeql-development-mcp-server/sessions/8abb21bb-8877-4628-90da-36ffc8eeb742 Co-authored-by: data-douser <70299490+data-douser@users.noreply.github.com> * feat: add client integration test for sarif_diff_by_commits - Add sarif_diff_by_commits case to Go params.go test runner - Add Go unit test for param resolution (params_test.go) - Create file_level_classification integration test fixture with: - SARIF with 3 results across 2 rules - HEAD..HEAD ref range (empty diff → all pre-existing) - Assertions validating totalNew=0, totalPreExisting=3 - before/after directories with SARIF and monitoring state Agent-Logs-Url: https://github.com/advanced-security/codeql-development-mcp-server/sessions/edb1fae4-1f49-44f9-af31-71483b674da7 Co-authored-by: data-douser <70299490+data-douser@users.noreply.github.com> * Apply lint/format & rebuild server/dist/ * [UPDATE PRIMITIVE] `sarif_diff_by_commits` — refRange validation, file path improvement, test mock fix (#242) * Initial plan * Fix PR #236 review comments: refRange validation, ClassifiedResult.file, test mocking 1. Validate refRange in sarif_diff_by_commits to reject strings starting with '-' or containing whitespace (prevents git option injection). 2. Use matchingDiff.path for ClassifiedResult.file when a diff match exists, falling back to normalizeUri(uri) only for unmatched results (produces repo-relative paths instead of long file:// URI paths). 3. Replace vi.doMock with module-scope vi.mock + shared mockExecuteCLICommand to prevent module-cache flakiness in sarif_diff_by_commits handler tests. Agent-Logs-Url: https://github.com/advanced-security/codeql-development-mcp-server/sessions/1960960b-9658-44b5-87d8-bc29cc55a5ef Co-authored-by: data-douser <70299490+data-douser@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: data-douser <70299490+data-douser@users.noreply.github.com> * Extend client tests -> sarif_diff_by_commits tool * fix: address unresolved PR review comments - Fix deletion-only hunk misclassification in line-level granularity by adding hunksParsed flag to DiffFileEntry; parseGitDiffOutput sets it when @@ headers are seen, and diffSarifByCommits uses it to distinguish "no hunk info" from "deletion-only" diffs - Precompute normalized diff paths once before the results loop, removing the unused diffPathMatchesSarifUri wrapper - Migrate all params_test.go from t.TempDir() to project-local .tmp/ - Add regression tests for deletion-only diffs in unit and handler tests --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: data-douser <70299490+data-douser@users.noreply.github.com> Co-authored-by: Nathan Randall <data-douser@github.com>
1 parent 289d860 commit 0342edf

File tree

21 files changed

+1565
-29
lines changed

21 files changed

+1565
-29
lines changed

‎client/integration-tests/primitives/tools/sarif_diff_by_commits/file_level_classification/README.md‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
# Integration Test: sarif_diff_by_commits - file_level_classification
2+
3+
## Purpose
4+
5+
Validates that the `sarif_diff_by_commits` tool correctly partitions
6+
SARIF results into "new" vs "pre-existing" based on file-level overlap
7+
with a git diff. Uses `HEAD..HEAD` (empty diff) so all results are
8+
classified as pre-existing.
9+
10+
## Inputs
11+
12+
- `results.sarif`: SARIF with 3 results across 2 rules in 3 files
13+
- `refRange`: `HEAD..HEAD` (produces an empty diff)
14+
- `granularity`: `file`
15+
16+
## Expected Behavior
17+
18+
Returns structured output with all 3 results in `preExistingResults`
19+
and 0 results in `newResults`, since the empty diff has no changed files.

‎client/integration-tests/primitives/tools/sarif_diff_by_commits/file_level_classification/after/monitoring-state.json‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
{
2+
"toolName": "sarif_diff_by_commits",
3+
"success": true,
4+
"description": "Successfully classified all results as pre-existing with empty diff"
5+
}

‎client/integration-tests/primitives/tools/sarif_diff_by_commits/file_level_classification/after/results.sarif‎

Lines changed: 107 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,107 @@
1+
{
2+
"version": "2.1.0",
3+
"$schema": "https://json.schemastore.org/sarif-2.1.0.json",
4+
"runs": [
5+
{
6+
"tool": {
7+
"driver": {
8+
"name": "CodeQL",
9+
"version": "2.20.4",
10+
"rules": [
11+
{
12+
"id": "js/sql-injection",
13+
"name": "js/sql-injection",
14+
"shortDescription": {
15+
"text": "Database query built from user-controlled sources"
16+
},
17+
"properties": {
18+
"tags": ["security"],
19+
"kind": "path-problem",
20+
"precision": "high",
21+
"security-severity": "8.8"
22+
}
23+
},
24+
{
25+
"id": "js/xss",
26+
"name": "js/xss",
27+
"shortDescription": {
28+
"text": "Cross-site scripting"
29+
},
30+
"properties": {
31+
"tags": ["security"],
32+
"kind": "path-problem",
33+
"precision": "high",
34+
"security-severity": "6.1"
35+
}
36+
}
37+
]
38+
}
39+
},
40+
"results": [
41+
{
42+
"ruleId": "js/sql-injection",
43+
"ruleIndex": 0,
44+
"message": {
45+
"text": "SQL injection from user input."
46+
},
47+
"locations": [
48+
{
49+
"physicalLocation": {
50+
"artifactLocation": {
51+
"uri": "src/db.js"
52+
},
53+
"region": {
54+
"startLine": 42,
55+
"startColumn": 5,
56+
"endColumn": 38
57+
}
58+
}
59+
}
60+
]
61+
},
62+
{
63+
"ruleId": "js/sql-injection",
64+
"ruleIndex": 0,
65+
"message": {
66+
"text": "SQL injection from request body."
67+
},
68+
"locations": [
69+
{
70+
"physicalLocation": {
71+
"artifactLocation": {
72+
"uri": "src/api.js"
73+
},
74+
"region": {
75+
"startLine": 15,
76+
"startColumn": 3,
77+
"endColumn": 40
78+
}
79+
}
80+
}
81+
]
82+
},
83+
{
84+
"ruleId": "js/xss",
85+
"ruleIndex": 1,
86+
"message": {
87+
"text": "XSS vulnerability."
88+
},
89+
"locations": [
90+
{
91+
"physicalLocation": {
92+
"artifactLocation": {
93+
"uri": "src/views.js"
94+
},
95+
"region": {
96+
"startLine": 30,
97+
"startColumn": 10,
98+
"endColumn": 50
99+
}
100+
}
101+
}
102+
]
103+
}
104+
]
105+
}
106+
]
107+
}

‎client/integration-tests/primitives/tools/sarif_diff_by_commits/file_level_classification/before/monitoring-state.json‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
{
2+
"toolName": "sarif_diff_by_commits",
3+
"expectedSuccess": true,
4+
"description": "Test sarif_diff_by_commits classifies all results as pre-existing with empty diff"
5+
}

‎client/integration-tests/primitives/tools/sarif_diff_by_commits/file_level_classification/before/results.sarif‎

Lines changed: 107 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,107 @@
1+
{
2+
"version": "2.1.0",
3+
"$schema": "https://json.schemastore.org/sarif-2.1.0.json",
4+
"runs": [
5+
{
6+
"tool": {
7+
"driver": {
8+
"name": "CodeQL",
9+
"version": "2.20.4",
10+
"rules": [
11+
{
12+
"id": "js/sql-injection",
13+
"name": "js/sql-injection",
14+
"shortDescription": {
15+
"text": "Database query built from user-controlled sources"
16+
},
17+
"properties": {
18+
"tags": ["security"],
19+
"kind": "path-problem",
20+
"precision": "high",
21+
"security-severity": "8.8"
22+
}
23+
},
24+
{
25+
"id": "js/xss",
26+
"name": "js/xss",
27+
"shortDescription": {
28+
"text": "Cross-site scripting"
29+
},
30+
"properties": {
31+
"tags": ["security"],
32+
"kind": "path-problem",
33+
"precision": "high",
34+
"security-severity": "6.1"
35+
}
36+
}
37+
]
38+
}
39+
},
40+
"results": [
41+
{
42+
"ruleId": "js/sql-injection",
43+
"ruleIndex": 0,
44+
"message": {
45+
"text": "SQL injection from user input."
46+
},
47+
"locations": [
48+
{
49+
"physicalLocation": {
50+
"artifactLocation": {
51+
"uri": "src/db.js"
52+
},
53+
"region": {
54+
"startLine": 42,
55+
"startColumn": 5,
56+
"endColumn": 38
57+
}
58+
}
59+
}
60+
]
61+
},
62+
{
63+
"ruleId": "js/sql-injection",
64+
"ruleIndex": 0,
65+
"message": {
66+
"text": "SQL injection from request body."
67+
},
68+
"locations": [
69+
{
70+
"physicalLocation": {
71+
"artifactLocation": {
72+
"uri": "src/api.js"
73+
},
74+
"region": {
75+
"startLine": 15,
76+
"startColumn": 3,
77+
"endColumn": 40
78+
}
79+
}
80+
}
81+
]
82+
},
83+
{
84+
"ruleId": "js/xss",
85+
"ruleIndex": 1,
86+
"message": {
87+
"text": "XSS vulnerability."
88+
},
89+
"locations": [
90+
{
91+
"physicalLocation": {
92+
"artifactLocation": {
93+
"uri": "src/views.js"
94+
},
95+
"region": {
96+
"startLine": 30,
97+
"startColumn": 10,
98+
"endColumn": 50
99+
}
100+
}
101+
}
102+
]
103+
}
104+
]
105+
}
106+
]
107+
}

‎client/integration-tests/primitives/tools/sarif_diff_by_commits/file_level_classification/test-config.json‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
{
2+
"toolName": "sarif_diff_by_commits",
3+
"arguments": {
4+
"refRange": "HEAD..HEAD",
5+
"granularity": "file"
6+
},
7+
"assertions": {
8+
"responseContains": [
9+
"\"granularity\": \"file\"",
10+
"\"preExistingResults\"",
11+
"\"newResults\"",
12+
"\"totalPreExisting\": 3",
13+
"\"totalNew\": 0",
14+
"\"totalResults\": 3"
15+
]
16+
}
17+
}

‎client/integration-tests/primitives/tools/sarif_diff_by_commits/line_level_classification/README.md‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
# Integration Test: sarif_diff_by_commits - line_level_classification
2+
3+
## Purpose
4+
5+
Validates that the `sarif_diff_by_commits` tool correctly handles
6+
line-level granularity classification. Uses `HEAD..HEAD` (empty diff)
7+
so all results are classified as pre-existing regardless of their
8+
line positions.
9+
10+
## Inputs
11+
12+
- `results.sarif`: SARIF with 3 results across 2 rules in 3 files
13+
- `refRange`: `HEAD..HEAD` (produces an empty diff)
14+
- `granularity`: `line`
15+
16+
## Expected Behavior
17+
18+
Returns structured output with `"granularity": "line"`, all 3 results
19+
in `preExistingResults`, and 0 results in `newResults`, since the
20+
empty diff has no changed files or hunks to match against.

‎client/integration-tests/primitives/tools/sarif_diff_by_commits/line_level_classification/after/monitoring-state.json‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
{
2+
"toolName": "sarif_diff_by_commits",
3+
"success": true,
4+
"description": "Successfully classified all results as pre-existing with empty diff (line granularity)"
5+
}

0 commit comments

Comments
 (0)