[UPDATE PRIMITIVE] sarif_diff_by_commits — refRange validation, file path improvement, test mock fix

[Copilot]

📝 Update Information

Primitive Details

• Type: Tool
• Name: sarif_diff_by_commits
• Update Category: Bug Fix (review feedback from PR [NEW PRIMITIVE] sarif_diff_by_commits — SARIF-to-git-diff correlation tool #236)

⚠️ CRITICAL: PR SCOPE VALIDATION

✅ ALLOWED FILES:

• Server implementation files (server/src/**/*.ts)
• Modified supporting library files (server/src/lib/*.ts)
• Updated test files (server/test/**/*.ts)

🚫 FORBIDDEN FILES: None included.

🛑 MANDATORY PR VALIDATION CHECKLIST

• ONLY server implementation files are included
• NO temporary or output files are included
• NO unrelated configuration files are included
• ALL existing tests continue to pass
• NEW functionality is properly tested

---

• Impact Scope: Localized
• Breaking Changes: No
• API Compatibility: Enhanced (new validation, improved output)
• Performance Impact: Neutral

🎯 Changes Description

Addresses three review comments from PR #236 review.

Current Behavior

1. refRange passed directly to git diff without validation — a value like --option is interpreted as a git flag
2. ClassifiedResult.file always uses normalizeUri(uri), producing home/user/project/src/db.js for file:/// URIs even when a diff match provides the repo-relative path
3. Tests use vi.doMock per-test for cli-executor, but the dynamic import() in the handler caches the module after the first call — later tests may silently reuse stale mocks

Updated Behavior

1. refRange is validated before use: rejects values starting with - or containing whitespace
2. ClassifiedResult.file uses matchingDiff.path when matched (repo-relative), falls back to normalizeUri(uri) only for unmatched results
3. Single module-scope vi.mock with shared mockExecuteCLICommand configured per-test

Motivation

Review feedback on PR #236.

🔄 Before vs. After Comparison

Functionality Changes

// BEFORE: refRange passed unchecked
const gitArgs = ['diff', '--unified=0', '--diff-filter=ACMR', '--no-color', refRange];

// AFTER: validated first
if (/^\s*-/.test(refRange) || /\s/.test(refRange)) {
  return { content: [{ type: 'text', text: 'Invalid refRange: must not start with "-" or contain whitespace.' }] };
}

// BEFORE: always normalizeUri
file: normalizeUri(uri),  // "home/user/project/src/db.js" for file:/// URIs

// AFTER: prefer diff path when matched
file: matchingDiff ? matchingDiff.path : normalizeUri(uri),  // "src/db.js"

🧪 Testing & Validation

Test Coverage Updates

• Existing Tests: All 1380 tests continue to pass
• New Test Cases: 3 new tests (refRange dash, refRange whitespace, unmatched URI fallback)
• Regression Tests: Enhanced file:// URI test to assert diff path is used
• Edge Case Tests: Covers option injection and whitespace in refRange

Test Results

• Unit Tests: All pass (110/110 SARIF tests)
• Integration Tests: N/A (no changes to integration fixtures)

📋 Implementation Details

Files Modified

• Core Implementation: server/src/tools/sarif-tools.ts — refRange validation
• Supporting Libraries: server/src/lib/sarif-utils.ts — ClassifiedResult.file path logic
• Tests: server/test/src/tools/sarif-tools.test.ts — mock pattern + validation tests
• Tests: server/test/src/lib/sarif-utils.test.ts — file path assertions

Dependencies

• No New Dependencies

🔗 References

Related Issues/PRs

• Related PRs: [NEW PRIMITIVE] sarif_diff_by_commits — SARIF-to-git-diff correlation tool #236

🚀 Compatibility & Migration

Backward Compatibility

• Fully Compatible: No breaking changes

ClassifiedResult.file now returns shorter repo-relative paths when a diff match exists. Consumers comparing against normalizeUri() output for matched results may see different values — but the new values are strictly more useful (and what the PR description already documented as the expected format).

API Evolution

• Better Error Messages: Clear rejection of invalid refRange
• Improved Responses: Repo-relative file paths in classified results
• Maintained Contracts: Core API contracts preserved

👥 Review Guidelines

For Reviewers

• ⚠️ SCOPE COMPLIANCE: Only 4 server files changed
• ⚠️ NO UNRELATED FILES: Clean diff
• ⚠️ BACKWARD COMPATIBILITY: Existing functionality preserved

Testing Instructions

npm run build-and-test

# Targeted tests
npx vitest run server/test/src/tools/sarif-tools.test.ts server/test/src/lib/sarif-utils.test.ts

📊 Impact Assessment

Server Impact

• Startup Time: No impact
• Runtime Stability: Improved (rejects malformed input early)
• Resource Usage: No change
• Concurrent Usage: Safe

🔄 Deployment Strategy

• Safe Deployment: Additive validation + improved output only
• Rollback Plan: Revert single commit

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

• update.code.visualstudio.com
  • Triggering command: /opt/hostedtoolcache/node/24.13.0/x64/bin/node node scripts/download-vscode.js -tests/primitive--unified=0 (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

• Configure Actions setup steps to set up my environment, which run before the firewall is enabled
• Add the appropriate URLs or hosts to the custom allowlist in this repository's Copilot coding agent settings (admins only)