fix: address false negatives in diff matching and enhance hunk processing
- Updated the root cause analysis for false negatives in diff matching, clarifying that context lines are discarded during matching. - Improved the `structuredPatch()` method to merge hunks based on a new threshold, enhancing accuracy in matching snippets. - Implemented reconstruction of "old side" and "new side" content from hunks, including context lines to prevent false negatives. - Enhanced the `CliStatusBanner` component to improve user feedback on CLI authentication status. - Updated the change review slice to incorporate file chunk counts for better hunk decision handling.
This commit is contained in:
parent
960bd2fa30
commit
43dd94cd2c
4 changed files with 90 additions and 57 deletions
|
|
@ -22,36 +22,58 @@ Source: 3 parallel research agents (~260k tokens total)
|
||||||
|
|
||||||
### A1. FALSE NEGATIVES (критичнее)
|
### A1. FALSE NEGATIVES (критичнее)
|
||||||
|
|
||||||
**Root cause**: Различие whitespace между snippet и hunk.
|
**Root cause (уточнён Round 3.1)**: НЕ whitespace (предыдущий анализ был неверен — Edit tool хранит точный текст с indentation). Реальная причина: **context lines** в hunk отбрасываются при matching.
|
||||||
|
|
||||||
- `ChangeExtractorService` хранит `oldString`/`newString` как есть из Edit tool_use — **без leading whitespace**
|
**Механизм**:
|
||||||
- `structuredPatch()` генерирует hunk lines с indentation из файла: `" const x = 1;"`
|
- `HunkSnippetMatcher` берёт только `+` и `-` строки из хунка, отбрасывая context (` ` prefix)
|
||||||
- `.slice(1)` убирает только `+`/`-` prefix, оставляя indentation
|
- `removedContent` = join только `-` строк → контекстные строки МЕЖДУ изменёнными строками теряются
|
||||||
- `.includes()` сравнивает `"const x"` vs `" const x"` → **не находит**
|
- Snippet `oldString` содержит ВСЕ строки (включая context), т.к. это точная подстрока файла
|
||||||
|
- `includes()` в обе стороны фейлится: ни `removedContent ⊂ oldString`, ни наоборот
|
||||||
|
|
||||||
**Конкретный пример**:
|
**Concrete proof** (из ресёрча):
|
||||||
```typescript
|
```typescript
|
||||||
// Файл с 3 Edit-ами в строках, разделённых < 3 строками:
|
// Claude's Edit:
|
||||||
const [state, setState] = useState(0); // ← Edit 1
|
// old_string = "interface UserConfig {\n name: string;\n age: number;\n email: string;\n active: boolean;\n premium: boolean;\n}"
|
||||||
const [data, setData] = useState(null); // ← Edit 2
|
// new_string = "interface UserSettings {\n name: string;\n age: number;\n email: string;\n active: boolean;\n isPremium: boolean;\n}"
|
||||||
const [loading, setLoading] = useState(false); // ← Edit 3
|
|
||||||
|
// structuredPatch() hunk:
|
||||||
|
// -interface UserConfig { ← removed
|
||||||
|
// name: string; ← CONTEXT (discarded!)
|
||||||
|
// age: number; ← CONTEXT (discarded!)
|
||||||
|
// email: string; ← CONTEXT (discarded!)
|
||||||
|
// active: boolean; ← CONTEXT (discarded!)
|
||||||
|
// - premium: boolean; ← removed
|
||||||
|
// +interface UserSettings { ← added
|
||||||
|
// + isPremium: boolean; ← added
|
||||||
|
|
||||||
|
// removedContent = "interface UserConfig {\n premium: boolean;"
|
||||||
|
// oldString = "interface UserConfig {\n name: string;\n age: number;\n email: string;\n active: boolean;\n premium: boolean;\n}"
|
||||||
|
// removedContent.includes(oldString) → NO
|
||||||
|
// oldString.includes(removedContent) → NO (context lines break contiguity)
|
||||||
|
// ❌ FALSE NEGATIVE — snippet не матчится к своему хунку
|
||||||
```
|
```
|
||||||
|
|
||||||
`structuredPatch()` сливает их в ОДИН хунк (context window = 3 строки).
|
**`structuredPatch()` merge threshold**: `context * 2` = 8 строк (default context=4). Хунки мержатся если gap ≤ 8 строк.
|
||||||
|
|
||||||
- Snippet: `oldString = "const [state, setState] = useState(0);"`
|
**Частота**: ВЫСОКАЯ. Любой Edit где Claude захватывает блок с неизменёнными строками внутри:
|
||||||
- Hunk removed: `" const [state, setState] = useState(0);"` (с отступом)
|
- Переименование interface/class + изменение полей
|
||||||
- `includes()` → false
|
- Смена параметров функции + изменение body
|
||||||
|
- Конфигурационные объекты (часть полей меняется, часть нет)
|
||||||
|
|
||||||
**Решение**: Нормализация whitespace при matching:
|
**Решение**: Реконструировать "old side" и "new side" хунка включая context lines:
|
||||||
```typescript
|
```typescript
|
||||||
const normalize = (s: string) =>
|
// Вместо только +/- строк:
|
||||||
s.split('\n').map(l => l.trim()).filter(l => l).join('\n');
|
const oldSideContent = hunk.lines
|
||||||
|
.filter(l => l.startsWith(' ') || l.startsWith('-'))
|
||||||
|
.map(l => l.slice(1)).join('\n');
|
||||||
|
const newSideContent = hunk.lines
|
||||||
|
.filter(l => l.startsWith(' ') || l.startsWith('+'))
|
||||||
|
.map(l => l.slice(1)).join('\n');
|
||||||
|
// oldSideContent.includes(snippet.oldString) → TRUE ✓
|
||||||
|
// newSideContent.includes(snippet.newString) → TRUE ✓
|
||||||
```
|
```
|
||||||
|
|
||||||
**Риск**: минимальный — whitespace-only diff в Edit почти невозможен (Claude всегда меняет контент).
|
**Уверенность**: 9.5/10 что реальный баг. 9/10 что fix через old/new side reconstruction работает.
|
||||||
|
|
||||||
**Оценка**: фиксит ~90% false negatives от merged hunks.
|
|
||||||
|
|
||||||
### A2. FALSE POSITIVES
|
### A2. FALSE POSITIVES
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -37,17 +37,22 @@ export class HunkSnippetMatcher {
|
||||||
const hunk = patch.hunks[hunkIdx];
|
const hunk = patch.hunks[hunkIdx];
|
||||||
const snippetSet = new Set<number>();
|
const snippetSet = new Set<number>();
|
||||||
|
|
||||||
// Extract added/removed content from hunk
|
// Reconstruct old/new side of hunk INCLUDING context lines.
|
||||||
const addedLines = hunk.lines.filter((l) => l.startsWith('+')).map((l) => l.slice(1));
|
// Context lines (` ` prefix) are critical — without them, snippets whose
|
||||||
const removedLines = hunk.lines.filter((l) => l.startsWith('-')).map((l) => l.slice(1));
|
// oldString spans unchanged lines between changed lines can't be matched.
|
||||||
const addedContent = addedLines.join('\n');
|
const oldSideContent = hunk.lines
|
||||||
const removedContent = removedLines.join('\n');
|
.filter((l) => !l.startsWith('+'))
|
||||||
|
.map((l) => l.slice(1))
|
||||||
|
.join('\n');
|
||||||
|
const newSideContent = hunk.lines
|
||||||
|
.filter((l) => !l.startsWith('-'))
|
||||||
|
.map((l) => l.slice(1))
|
||||||
|
.join('\n');
|
||||||
|
|
||||||
for (let sIdx = 0; sIdx < snippets.length; sIdx++) {
|
for (let sIdx = 0; sIdx < snippets.length; sIdx++) {
|
||||||
const snippet = snippets[sIdx];
|
const snippet = snippets[sIdx];
|
||||||
|
|
||||||
// Content overlap: check if snippet's strings appear in hunk's diff content
|
if (this.hasContentOverlap(snippet, oldSideContent, newSideContent)) {
|
||||||
if (this.hasContentOverlap(snippet, addedContent, removedContent)) {
|
|
||||||
snippetSet.add(sIdx);
|
snippetSet.add(sIdx);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
@ -116,40 +121,30 @@ export class HunkSnippetMatcher {
|
||||||
// ── Private helpers ──
|
// ── Private helpers ──
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Check if a snippet's content overlaps with hunk's added/removed content.
|
* Check if a snippet's content overlaps with a hunk's reconstructed file ranges.
|
||||||
|
*
|
||||||
|
* @param hunkOldSide — reconstructed original file text within hunk range (context + removed lines)
|
||||||
|
* @param hunkNewSide — reconstructed modified file text within hunk range (context + added lines)
|
||||||
*/
|
*/
|
||||||
private hasContentOverlap(
|
private hasContentOverlap(
|
||||||
snippet: SnippetDiff,
|
snippet: SnippetDiff,
|
||||||
hunkAddedContent: string,
|
hunkOldSide: string,
|
||||||
hunkRemovedContent: string
|
hunkNewSide: string
|
||||||
): boolean {
|
): boolean {
|
||||||
// Skip empty snippets
|
|
||||||
if (!snippet.newString && !snippet.oldString) return false;
|
if (!snippet.newString && !snippet.oldString) return false;
|
||||||
|
|
||||||
if (snippet.type === 'write-new' || snippet.type === 'write-update') {
|
if (snippet.type === 'write-new' || snippet.type === 'write-update') {
|
||||||
// For Write: check if hunk's added content is a substring of snippet's newString
|
// For Write: snippet.newString is the full file content — check if hunk's new side is within it
|
||||||
if (snippet.newString && hunkAddedContent) {
|
if (snippet.newString && hunkNewSide) {
|
||||||
return snippet.newString.includes(hunkAddedContent);
|
return snippet.newString.includes(hunkNewSide);
|
||||||
}
|
}
|
||||||
return false;
|
return false;
|
||||||
}
|
}
|
||||||
|
|
||||||
// For Edit/MultiEdit: check bidirectional overlap
|
// For Edit/MultiEdit: check if snippet falls within hunk's file range
|
||||||
const matchesNew = snippet.newString
|
const matchesOld = snippet.oldString ? hunkOldSide.includes(snippet.oldString) : false;
|
||||||
? hunkAddedContent.includes(snippet.newString) || snippet.newString.includes(hunkAddedContent)
|
const matchesNew = snippet.newString ? hunkNewSide.includes(snippet.newString) : false;
|
||||||
: false;
|
|
||||||
|
|
||||||
const matchesOld = snippet.oldString
|
return matchesOld || matchesNew;
|
||||||
? hunkRemovedContent.includes(snippet.oldString) ||
|
|
||||||
snippet.oldString.includes(hunkRemovedContent)
|
|
||||||
: false;
|
|
||||||
|
|
||||||
// Both directions match = high confidence
|
|
||||||
if (matchesNew && matchesOld) return true;
|
|
||||||
|
|
||||||
// Single direction match = acceptable for Edit
|
|
||||||
if (matchesNew || matchesOld) return true;
|
|
||||||
|
|
||||||
return false;
|
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -161,9 +161,18 @@ export const CliStatusBanner = (): React.JSX.Element | null => {
|
||||||
const [showLoginTerminal, setShowLoginTerminal] = useState(false);
|
const [showLoginTerminal, setShowLoginTerminal] = useState(false);
|
||||||
|
|
||||||
useEffect(() => {
|
useEffect(() => {
|
||||||
if (isElectron) {
|
if (!isElectron) return;
|
||||||
void fetchCliStatus();
|
|
||||||
}
|
void fetchCliStatus();
|
||||||
|
|
||||||
|
const interval = setInterval(
|
||||||
|
() => {
|
||||||
|
void fetchCliStatus();
|
||||||
|
},
|
||||||
|
10 * 60 * 1000
|
||||||
|
);
|
||||||
|
|
||||||
|
return () => clearInterval(interval);
|
||||||
}, [isElectron, fetchCliStatus]);
|
}, [isElectron, fetchCliStatus]);
|
||||||
|
|
||||||
const handleInstall = useCallback(() => {
|
const handleInstall = useCallback(() => {
|
||||||
|
|
@ -447,6 +456,11 @@ export const CliStatusBanner = (): React.JSX.Element | null => {
|
||||||
<span className="text-sm" style={{ color: 'var(--color-text)' }}>
|
<span className="text-sm" style={{ color: 'var(--color-text)' }}>
|
||||||
Claude CLI v{cliStatus.installedVersion ?? 'unknown'}
|
Claude CLI v{cliStatus.installedVersion ?? 'unknown'}
|
||||||
</span>
|
</span>
|
||||||
|
{cliStatus.authLoggedIn && (
|
||||||
|
<span className="text-xs" style={{ color: '#4ade80' }}>
|
||||||
|
Authenticated
|
||||||
|
</span>
|
||||||
|
)}
|
||||||
{cliStatus.updateAvailable && cliStatus.latestVersion && (
|
{cliStatus.updateAvailable && cliStatus.latestVersion && (
|
||||||
<span className="text-xs" style={{ color: '#60a5fa' }}>
|
<span className="text-xs" style={{ color: '#60a5fa' }}>
|
||||||
→ v{cliStatus.latestVersion}
|
→ v{cliStatus.latestVersion}
|
||||||
|
|
|
||||||
|
|
@ -540,7 +540,7 @@ export const createChangeReviewSlice: StateCreator<AppState, [], [], ChangeRevie
|
||||||
}
|
}
|
||||||
|
|
||||||
// Build FileReviewDecision[] from hunkDecisions/fileDecisions
|
// Build FileReviewDecision[] from hunkDecisions/fileDecisions
|
||||||
const { hunkDecisions, fileDecisions, activeChangeSet } = get();
|
const { hunkDecisions, fileDecisions, fileChunkCounts, activeChangeSet } = get();
|
||||||
if (!activeChangeSet) {
|
if (!activeChangeSet) {
|
||||||
set({ applying: false });
|
set({ applying: false });
|
||||||
return;
|
return;
|
||||||
|
|
@ -552,7 +552,8 @@ export const createChangeReviewSlice: StateCreator<AppState, [], [], ChangeRevie
|
||||||
const fileDecision = fileDecisions[file.filePath] ?? 'pending';
|
const fileDecision = fileDecisions[file.filePath] ?? 'pending';
|
||||||
const hunkDecs: Record<number, HunkDecision> = {};
|
const hunkDecs: Record<number, HunkDecision> = {};
|
||||||
|
|
||||||
for (let i = 0; i < file.snippets.length; i++) {
|
const count = getFileHunkCount(file.filePath, file.snippets.length, fileChunkCounts);
|
||||||
|
for (let i = 0; i < count; i++) {
|
||||||
const key = `${file.filePath}:${i}`;
|
const key = `${file.filePath}:${i}`;
|
||||||
hunkDecs[i] = hunkDecisions[key] ?? 'pending';
|
hunkDecs[i] = hunkDecisions[key] ?? 'pending';
|
||||||
}
|
}
|
||||||
|
|
@ -599,7 +600,7 @@ export const createChangeReviewSlice: StateCreator<AppState, [], [], ChangeRevie
|
||||||
taskId?: string,
|
taskId?: string,
|
||||||
memberName?: string
|
memberName?: string
|
||||||
) => {
|
) => {
|
||||||
const { hunkDecisions, fileDecisions, activeChangeSet } = get();
|
const { hunkDecisions, fileDecisions, fileChunkCounts, activeChangeSet } = get();
|
||||||
if (!activeChangeSet) return;
|
if (!activeChangeSet) return;
|
||||||
|
|
||||||
const file = activeChangeSet.files.find((f) => f.filePath === filePath);
|
const file = activeChangeSet.files.find((f) => f.filePath === filePath);
|
||||||
|
|
@ -607,7 +608,8 @@ export const createChangeReviewSlice: StateCreator<AppState, [], [], ChangeRevie
|
||||||
|
|
||||||
const fileDecision = fileDecisions[filePath] ?? 'pending';
|
const fileDecision = fileDecisions[filePath] ?? 'pending';
|
||||||
const hunkDecs: Record<number, HunkDecision> = {};
|
const hunkDecs: Record<number, HunkDecision> = {};
|
||||||
for (let i = 0; i < file.snippets.length; i++) {
|
const count = getFileHunkCount(filePath, file.snippets.length, fileChunkCounts);
|
||||||
|
for (let i = 0; i < count; i++) {
|
||||||
hunkDecs[i] = hunkDecisions[`${filePath}:${i}`] ?? 'pending';
|
hunkDecs[i] = hunkDecisions[`${filePath}:${i}`] ?? 'pending';
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue