[NEW PRIMITIVE] sarif_diff_by_commits — SARIF-to-git-diff correlation tool#236
[NEW PRIMITIVE] sarif_diff_by_commits — SARIF-to-git-diff correlation tool#236data-douser merged 7 commits intonextfrom
sarif_diff_by_commits — SARIF-to-git-diff correlation tool#236Conversation
sarif_diff_by_commits — SARIF-to-git-diff correlation tool
data-douser
left a comment
There was a problem hiding this comment.
@copilot Any new MCP tool must have at least one covering client integration test in order to validate (or invalidate) the proper functionality of the new tool. Our new sarif_diff_by_commits needs to be much more extensively tested.
Added in 9723dbf: Go integration test runner support:
Client integration test fixture (
Combined with the existing 14
|
29b775c to
7a7f8df
Compare
There was a problem hiding this comment.
Pull request overview
Adds a new SARIF analysis MCP tool, sarif_diff_by_commits, to correlate SARIF findings with a git diff over a commit range and classify results as “new” vs “pre-existing”, with supporting docs and tests across server + client integration runner.
Changes:
- Implemented
diffSarifByCommits()(pure classification) and integrated it into a newsarif_diff_by_commitsMCP tool that shells out togit diff --unified=0. - Added server unit tests for classification logic and tool handler behavior, and updated SARIF tool registration expectations (7 → 8).
- Added client-side param injection support plus a new integration test fixture for file-level classification.
Show a summary per file
| File | Description |
|---|---|
| server/src/lib/sarif-utils.ts | Adds diff correlation types and diffSarifByCommits() classification logic. |
| server/src/tools/sarif-tools.ts | Registers the new tool, runs git diff, parses unified diff hunks, and returns classified JSON. |
| server/src/resources/server-tools.md | Documents the new SARIF tool in the server tools reference and workflow list. |
| server/test/src/lib/sarif-utils.test.ts | Adds unit tests for file/line granularity classification behavior. |
| server/test/src/tools/sarif-tools.test.ts | Updates tool registration count and adds handler tests for sarif_diff_by_commits. |
| server/dist/codeql-development-mcp-server.js | Updates bundled build artifact to include the new tool and utility code. |
| client/internal/testing/params.go | Adds sarif_diff_by_commits param construction (inject SARIF path + merge config args). |
| client/internal/testing/params_test.go | Adds a Go unit test verifying param injection/merge for the new tool. |
| client/integration-tests/primitives/tools/sarif_diff_by_commits/file_level_classification/test-config.json | Adds integration test config and response assertions. |
| client/integration-tests/primitives/tools/sarif_diff_by_commits/file_level_classification/README.md | Documents the integration test purpose and expectations. |
| client/integration-tests/primitives/tools/sarif_diff_by_commits/file_level_classification/before/results.sarif | Adds SARIF fixture input with 3 results in 3 files. |
| client/integration-tests/primitives/tools/sarif_diff_by_commits/file_level_classification/before/monitoring-state.json | Adds integration “before” monitoring state. |
| client/integration-tests/primitives/tools/sarif_diff_by_commits/file_level_classification/after/results.sarif | Mirrors expected SARIF fixture state post-run (unchanged file). |
| client/integration-tests/primitives/tools/sarif_diff_by_commits/file_level_classification/after/monitoring-state.json | Adds integration “after” monitoring state. |
Copilot's findings
- Files reviewed: 13/15 changed files
- Comments generated: 3
…le, 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>
…e 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>
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>
- 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>
…e 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>
8f055f5 to
be5096d
Compare
There was a problem hiding this comment.
Pull request overview
Adds a new SARIF triage tool (sarif_diff_by_commits) to correlate SARIF results with a git ref-range diff, classifying findings as “new” vs “pre-existing”, and integrates it across server, tests, docs, and client integration fixtures.
Changes:
- Implement
diffSarifByCommits()inserver/src/lib/sarif-utils.tsand wire a newsarif_diff_by_commitsMCP tool inserver/src/tools/sarif-tools.ts(runsgit diff --unified=0and parses hunks). - Add/extend server unit tests and tool-handler tests for the new primitive.
- Add client param-resolution support + Go unit test + an integration-test fixture for file-level classification.
Show a summary per file
| File | Description |
|---|---|
| server/src/lib/sarif-utils.ts | Adds diff correlation types and the pure diffSarifByCommits() classifier. |
| server/src/tools/sarif-tools.ts | Registers sarif_diff_by_commits and parses git diff output into hunks/files. |
| server/test/src/lib/sarif-utils.test.ts | Adds unit tests covering file/line granularity classification behaviors. |
| server/test/src/tools/sarif-tools.test.ts | Adds handler tests for sarif_diff_by_commits and updates tool registration count. |
| server/src/resources/server-tools.md | Documents the new tool in the SARIF tools table and workflow list. |
| server/dist/codeql-development-mcp-server.js | Updates bundled build artifact to include the new tool/utility. |
| client/internal/testing/params.go | Adds integration-runner param resolution for sarif_diff_by_commits. |
| client/internal/testing/params_test.go | Adds a Go unit test validating params injection/merge for the new tool. |
| client/integration-tests/primitives/tools/sarif_diff_by_commits/file_level_classification/test-config.json | Adds a fixture config and response assertions for file-level classification. |
| client/integration-tests/primitives/tools/sarif_diff_by_commits/file_level_classification/README.md | Documents intent/inputs/expected behavior of the integration test. |
| client/integration-tests/primitives/tools/sarif_diff_by_commits/file_level_classification/before/results.sarif | SARIF input fixture with 3 results across 3 files. |
| client/integration-tests/primitives/tools/sarif_diff_by_commits/file_level_classification/before/monitoring-state.json | Integration test monitoring metadata (before). |
| client/integration-tests/primitives/tools/sarif_diff_by_commits/file_level_classification/after/results.sarif | Post-run fixture (unchanged SARIF). |
| client/integration-tests/primitives/tools/sarif_diff_by_commits/file_level_classification/after/monitoring-state.json | Integration test monitoring metadata (after). |
Copilot's findings
- Files reviewed: 13/15 changed files
- Comments generated: 3
There was a problem hiding this comment.
Pull request overview
Adds a new SARIF triage tool (sarif_diff_by_commits) to correlate SARIF findings with a git diff across a commit ref range, enabling “new vs pre-existing” classification at file or line granularity.
Changes:
- Implement
diffSarifByCommits()(pure utility) and supporting diff/correlation types inserver/src/lib/sarif-utils.ts. - Register new MCP tool
sarif_diff_by_commitsinserver/src/tools/sarif-tools.ts, including git diff parsing and refRange validation. - Add server/unit/tool tests, update tool documentation, and extend client integration test harness + fixtures for the new tool.
Show a summary per file
| File | Description |
|---|---|
| server/src/lib/sarif-utils.ts | Adds diff/correlation types and the diffSarifByCommits() classifier utility. |
| server/src/tools/sarif-tools.ts | Registers sarif_diff_by_commits, runs git diff, parses hunks, and returns classification output. |
| server/src/resources/server-tools.md | Documents the new tool in the SARIF tools table and recommended workflow list. |
| server/test/src/lib/sarif-utils.test.ts | Adds unit tests covering file/line classification behavior for diffSarifByCommits(). |
| server/test/src/tools/sarif-tools.test.ts | Updates registration count and adds handler tests for sarif_diff_by_commits with mocked git output. |
| server/dist/codeql-development-mcp-server.js | Updates bundled build artifact to include the new tool and utility logic. |
| client/internal/testing/params.go | Adds parameter-resolution support for sarif_diff_by_commits integration tests (SARIF path injection + config merge). |
| client/internal/testing/params_test.go | Adds a unit test for sarif_diff_by_commits param building and introduces a project-local tmp helper. |
| client/integration-tests/primitives/tools/sarif_diff_by_commits/file_level_classification/test-config.json | New integration test config for file-level classification with an empty diff. |
| client/integration-tests/primitives/tools/sarif_diff_by_commits/file_level_classification/before/results.sarif | SARIF fixture input for file-level integration test. |
| client/integration-tests/primitives/tools/sarif_diff_by_commits/file_level_classification/before/monitoring-state.json | Monitoring metadata for file-level integration test. |
| client/integration-tests/primitives/tools/sarif_diff_by_commits/file_level_classification/after/results.sarif | Expected SARIF fixture in after-state for file-level integration test. |
| client/integration-tests/primitives/tools/sarif_diff_by_commits/file_level_classification/after/monitoring-state.json | Expected monitoring outcome for file-level integration test. |
| client/integration-tests/primitives/tools/sarif_diff_by_commits/file_level_classification/README.md | Explains file-level integration test intent and expectations. |
| client/integration-tests/primitives/tools/sarif_diff_by_commits/line_level_classification/test-config.json | New integration test config for line-level classification with an empty diff. |
| client/integration-tests/primitives/tools/sarif_diff_by_commits/line_level_classification/before/results.sarif | SARIF fixture input for line-level integration test. |
| client/integration-tests/primitives/tools/sarif_diff_by_commits/line_level_classification/before/monitoring-state.json | Monitoring metadata for line-level integration test. |
| client/integration-tests/primitives/tools/sarif_diff_by_commits/line_level_classification/after/results.sarif | Expected SARIF fixture in after-state for line-level integration test. |
| client/integration-tests/primitives/tools/sarif_diff_by_commits/line_level_classification/after/monitoring-state.json | Expected monitoring outcome for line-level integration test. |
| client/integration-tests/primitives/tools/sarif_diff_by_commits/line_level_classification/README.md | Explains line-level integration test intent and expectations. |
Copilot's findings
- Files reviewed: 19/21 changed files
- Comments generated: 4
- 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
There was a problem hiding this comment.
Pull request overview
Adds a new SARIF analysis MCP tool (sarif_diff_by_commits) that correlates SARIF results to git diff output, enabling “new vs pre-existing” triage at file or line granularity. This fits into the existing SARIF tool suite by extending sarif-utils with a pure classifier and wiring it into server/src/tools/sarif-tools.ts, plus test coverage and client integration fixtures.
Changes:
- Add
diffSarifByCommits()+ supporting types to classify SARIF results using parsedgit diffhunks. - Register the new
sarif_diff_by_commitstool and document it in the server tools resource. - Add unit/tool tests plus client param resolution and integration test fixtures for file/line modes.
Show a summary per file
| File | Description |
|---|---|
| server/src/lib/sarif-utils.ts | Adds diff-by-commits classification types and the diffSarifByCommits() implementation. |
| server/src/tools/sarif-tools.ts | Registers sarif_diff_by_commits, runs git diff, parses hunks, and returns classified output. |
| server/src/resources/server-tools.md | Documents the new SARIF tool in the tools table and workflow section. |
| server/test/src/lib/sarif-utils.test.ts | Adds unit tests covering file/line classification behavior and edge cases. |
| server/test/src/tools/sarif-tools.test.ts | Adds handler tests for sarif_diff_by_commits and updates registration count/expectations. |
| server/dist/codeql-development-mcp-server.js | Updates built server bundle to include the new tool and library logic. |
| client/internal/testing/params.go | Adds tool param injection/merging logic for sarif_diff_by_commits integration tests. |
| client/internal/testing/params_test.go | Updates temp dir strategy and adds a unit test validating params for sarif_diff_by_commits. |
| client/integration-tests/primitives/tools/sarif_diff_by_commits/file_level_classification/test-config.json | Adds integration test config/assertions for file-level classification on empty diff. |
| client/integration-tests/primitives/tools/sarif_diff_by_commits/file_level_classification/before/results.sarif | Adds SARIF fixture input for file-level integration test. |
| client/integration-tests/primitives/tools/sarif_diff_by_commits/file_level_classification/before/monitoring-state.json | Adds monitoring state fixture for file-level test “before” state. |
| client/integration-tests/primitives/tools/sarif_diff_by_commits/file_level_classification/after/results.sarif | Captures expected “after” filesystem state for file-level test. |
| client/integration-tests/primitives/tools/sarif_diff_by_commits/file_level_classification/after/monitoring-state.json | Captures expected success monitoring state for file-level test. |
| client/integration-tests/primitives/tools/sarif_diff_by_commits/file_level_classification/README.md | Documents the file-level integration test scenario and expectations. |
| client/integration-tests/primitives/tools/sarif_diff_by_commits/line_level_classification/test-config.json | Adds integration test config/assertions for line-level classification on empty diff. |
| client/integration-tests/primitives/tools/sarif_diff_by_commits/line_level_classification/before/results.sarif | Adds SARIF fixture input for line-level integration test. |
| client/integration-tests/primitives/tools/sarif_diff_by_commits/line_level_classification/before/monitoring-state.json | Adds monitoring state fixture for line-level test “before” state. |
| client/integration-tests/primitives/tools/sarif_diff_by_commits/line_level_classification/after/results.sarif | Captures expected “after” filesystem state for line-level test. |
| client/integration-tests/primitives/tools/sarif_diff_by_commits/line_level_classification/after/monitoring-state.json | Captures expected success monitoring state for line-level test. |
| client/integration-tests/primitives/tools/sarif_diff_by_commits/line_level_classification/README.md | Documents the line-level integration test scenario and expectations. |
Copilot's findings
- Files reviewed: 19/21 changed files
- Comments generated: 1
| // Find matching diff file using precomputed normalized paths | ||
| let matchingDiff: DiffFileEntry | undefined; | ||
| for (const { entry, normalized } of normalizedDiffEntries) { | ||
| if (normalized === normalizedUri || normalized.endsWith(normalizedUri) || normalizedUri.endsWith(normalized)) { | ||
| matchingDiff = entry; | ||
| break; | ||
| } | ||
| } |
Add report, assess, and apply subcommands to gh-ql-mcp-client for managing Code Scanning alerts across their full lifecycle. Phase 1 — code-scanning report: - Fetches alerts across all states (open, dismissed, fixed) to capture the complete alert lifecycle picture - Preserves dismissal metadata (reason, comment, by, at) for anti-churn - Groups alerts by rule with per-state counts - Output: <owner>_<repo>.cs-report.json Phase 2 — code-scanning assess: - Detects overlapping alerts across different rules at the same file:line - Flags churn risk when open alerts overlap dismissed ones - Recommends keep / keep-dismissed / keep-fixed / review / discard - Output: <owner>_<repo>.cs-assess.json Phase 3 — code-scanning apply: - Builds dismiss plan from assess report, executes via GitHub API - Supports --dry-run, --accept-all-changes, --accept-change-for-rule - Per-rule authorization when explicit rule filters are provided - Output: <owner>_<repo>.cs-apply.json Server changes: - Extract normalizedUrisMatch() from urisMatch() for precomputed paths (addresses unresolved PR #236 review comment) - Rebuild server dist with sarif-utils refactor
📝 Primitive Information
Primitive Details
sarif_diff_by_commits✅ ALLOWED FILES:
server/src/**/*.ts)server/src/tools/*.ts)server/test/**/*.ts)README.md, server docs)server/src/types/*.ts)server/src/lib/*.ts)package.json,tsconfig.json)client/integration-tests/**)client/internal/testing/*.go)🚫 FORBIDDEN FILES:
🛑 MANDATORY PR VALIDATION CHECKLIST
Primitive Metadata
{ sarifPath?, cacheKey?, refRange, repoPath?, granularity? }SarifDiffByCommitsResult— partitioned results with summary statistics🎯 Functionality Description
What This Primitive Does
Correlates SARIF analysis results with a git diff to classify each finding as "new" (introduced by the diff) or "pre-existing". Supports file-level and line-level granularity.
Previously, triaging SARIF results by branch required manually running
git diff --name-only, parsing SARIF location URIs, and cross-referencing. This tool automates that entire workflow in a single call.Use Cases
codeql_database_analyze, identify which findings were introduced on the current branch vs inherited from the basesarif_list_rulesandsarif_extract_ruleto produce focused reports on new findings onlyExample Usage
Example Input/Output
🧪 Implementation Details
Files Added/Modified
server/src/lib/sarif-utils.ts—diffSarifByCommits()pure function,DiffFileEntry,DiffGranularity,ClassifiedResult,SarifDiffByCommitsResulttypesserver/src/tools/sarif-tools.ts—registerSarifDiffByCommitsTool()+parseGitDiffOutput()helperserver/test/src/lib/sarif-utils.test.ts— 14 tests fordiffSarifByCommits()server/test/src/tools/sarif-tools.test.ts— 5 handler tests, registration count 7→8server/src/resources/server-tools.md— tool table + workflow sectionclient/internal/testing/params.go—sarif_diff_by_commitscase for param resolutionclient/internal/testing/params_test.go—TestBuildToolParams_SARIFDiffByCommitsWithConfigclient/integration-tests/primitives/tools/sarif_diff_by_commits/file_level_classification/— SARIF fixture, test-config.json with assertions, before/after directoriesArchitecture Integration
The implementation separates concerns into two layers:
diffSarifByCommits): No I/O, no side effects. Takes a SARIF document and pre-parsed diff entries, returns classified results. Directly unit-testable.loadSarif()) and git diff execution (viaexecuteCLICommand), delegates classification to the pure utility.parseGitDiffOutput()extracts file paths and hunk ranges fromgit diff --unified=0output, producingDiffFileEntry[]with{ path, hunks: [{ startLine, lineCount }] }.URI matching reuses the existing
normalizeUri()/urisMatch()infrastructure from sarif-utils sofile:///URIs, relative paths, and%SRCROOT%variants all work.Design Patterns
sarif_diff_runs,sarif_compare_alertsexecuteCLICommandfor git; no new depsgit diffcall; linear scan over results📋 Testing Coverage
Unit Tests
executeCLICommandTest Scenarios
file://URI normalizationfile_level_classification— validatesHEAD..HEADempty diff classifies all 3 results as pre-existingTest Files
server/test/src/lib/sarif-utils.test.ts— 14 new tests (76 total)server/test/src/tools/sarif-tools.test.ts— 5 new tests (31 total)client/internal/testing/params_test.go— 1 new Go unit testclient/integration-tests/primitives/tools/sarif_diff_by_commits/file_level_classification/— integration test fixture with SARIF, test-config.json (6 response assertions), README, before/after directories🔗 References
Related Implementation
sarif_diff_runs— same tool registration pattern, sameloadSarif()helpercomputeLocationOverlap— related URI normalization logic reused hereexecuteCLICommand— existing CLI executor withgitin the allowlistExternal References
server.tool()registrationphysicalLocation.artifactLocation.uri+region.startLinesarif_diff_runs,sarif_compare_alertsin same file🚀 Server Integration
Registration Details
Compatibility
executeCLICommandPerformance Considerations
git diffsubprocess call🔍 Quality Assurance
Code Quality
Validation Testing
file_level_classificationtest case; tool handler tested with mocked git outputSecurity Considerations
refRangepassed toexecuteCLICommandwhich usesexecFile(no shell)gitis in the allowlist; args sanitized by existing infrastructure👥 Review Guidelines
For Reviewers
Testing Instructions
📊 Impact Analysis
Server Impact
server.tool()registrationAI Assistant Benefits
sarif_list_rules,sarif_extract_rule,sarif_rule_to_markdownMonitoring & Reporting Integration
Maintenance Considerations
fileandlinegranularity🔄 Deployment Considerations
Rollout Strategy
Migration Notes
No migration required. The tool is a new addition to the existing SARIF tool suite.