Skip to content

Commit dfe75f1

Browse files
Unify Benchify resilience and batched str_replace refactors; simplify validation and streaming integration
Consolidates recent improvements to Benchify post-processing and batched str_replace execution. Reworks validateBenchifyResponse to use structural pattern match with clear business logic checks (no nested matches/continues), simplifies stream-parser integration, and expands tests for reliability. 🤖 Generated with Codebuff Co-Authored-By: Codebuff <noreply@codebuff.com>
1 parent 9dcb510 commit dfe75f1

File tree

4 files changed

+589
-118
lines changed

4 files changed

+589
-118
lines changed

backend/src/__tests__/process-str-replace.test.ts

Lines changed: 165 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,13 @@
1-
import { describe, expect, it } from 'bun:test'
1+
import * as envModule from '@codebuff/internal/env'
2+
import { describe, expect, it, spyOn } from 'bun:test'
23
import { applyPatch } from 'diff'
34

45
import { processStrReplace } from '../process-str-replace'
6+
import { mockFileContext } from './test-utils'
7+
import {
8+
executeBatchStrReplaces,
9+
benchifyCanFixLanguage,
10+
} from '../tools/batch-str-replace'
511

612
describe('processStrReplace', () => {
713
it('should replace exact string matches', async () => {
@@ -213,6 +219,25 @@ describe('processStrReplace', () => {
213219
}
214220
})
215221

222+
it('should handle replacement where old string equals new string', async () => {
223+
const initialContent = 'const x = 1;\nconst y = 2;\n'
224+
const oldStr = 'const y = 2;'
225+
const newStr = 'const y = 2;' // Same as old string
226+
227+
const result = await processStrReplace(
228+
'test.ts',
229+
[{ old: oldStr, new: newStr, allowMultiple: false }],
230+
Promise.resolve(initialContent),
231+
)
232+
233+
expect(result).not.toBeNull()
234+
expect('content' in result).toBe(true)
235+
if ('content' in result) {
236+
expect(result.content).toBe('const x = 1;\nconst y = 2;\n')
237+
expect(result.messages).toEqual([])
238+
}
239+
})
240+
216241
// New comprehensive tests for allowMultiple functionality
217242
describe('allowMultiple functionality', () => {
218243
it('should error when multiple occurrences exist and allowMultiple is false', async () => {
@@ -417,3 +442,142 @@ function test3() {
417442
)
418443
})
419444
})
445+
446+
// Tests for Benchify resilience
447+
describe('Benchify resilience', () => {
448+
describe('happy path', () => {
449+
it('should identify Benchify-supported file types correctly', () => {
450+
const testCases = [
451+
{ path: 'component.tsx', expected: true },
452+
{ path: 'utils.ts', expected: true },
453+
{ path: 'script.js', expected: true },
454+
{ path: 'styles.jsx', expected: true },
455+
{ path: 'README.md', expected: false },
456+
{ path: 'config.json', expected: false },
457+
{ path: 'styles.css', expected: false },
458+
{ path: 'index.html', expected: false },
459+
{ path: 'test.py', expected: false },
460+
]
461+
462+
for (const { path, expected } of testCases) {
463+
const result = benchifyCanFixLanguage(path)
464+
expect(result).toBe(expected)
465+
}
466+
})
467+
468+
it('should handle file extensions case sensitivity', () => {
469+
expect(benchifyCanFixLanguage('Component.TSX')).toBe(false) // Wrong case
470+
expect(benchifyCanFixLanguage('component.tsx')).toBe(true) // Correct case
471+
expect(benchifyCanFixLanguage('utils.TS')).toBe(false) // Wrong case
472+
expect(benchifyCanFixLanguage('utils.ts')).toBe(true) // Correct case
473+
})
474+
475+
it('should handle file paths with multiple dots', () => {
476+
expect(benchifyCanFixLanguage('component.test.tsx')).toBe(true)
477+
expect(benchifyCanFixLanguage('utils.spec.ts')).toBe(true)
478+
expect(benchifyCanFixLanguage('config.local.js')).toBe(true)
479+
expect(benchifyCanFixLanguage('styles.module.css')).toBe(false)
480+
})
481+
482+
it('should handle files without extensions', () => {
483+
expect(benchifyCanFixLanguage('Dockerfile')).toBe(false)
484+
expect(benchifyCanFixLanguage('Makefile')).toBe(false)
485+
expect(benchifyCanFixLanguage('README')).toBe(false)
486+
})
487+
})
488+
489+
it('should fall back gracefully when Benchify is disabled', async () => {
490+
// Test with no API key - spy on the env object directly
491+
spyOn(envModule, 'env', 'get').mockReturnValue({
492+
// Empty object simulates no BENCHIFY_API_KEY
493+
} as any)
494+
495+
const result = await executeBatchStrReplaces({
496+
deferredStrReplaces: [
497+
{
498+
toolCall: {
499+
toolName: 'str_replace' as const,
500+
toolCallId: 'test-call',
501+
input: {
502+
path: 'test.ts',
503+
replacements: [{ old: 'old', new: 'new', allowMultiple: false }],
504+
},
505+
},
506+
},
507+
],
508+
toolCalls: [],
509+
toolResults: [],
510+
ws: {} as any,
511+
fileContext: mockFileContext,
512+
agentStepId: 'test-step',
513+
clientSessionId: 'test-session',
514+
userInputId: 'test-input',
515+
onResponseChunk: () => {},
516+
state: { messages: [] },
517+
userId: 'test-user',
518+
})
519+
520+
// Should complete without error even when Benchify is unavailable
521+
expect(result).toBeUndefined() // Function returns void
522+
})
523+
524+
describe('Batch str_replace integration tests', () => {
525+
it('should handle empty deferred list without error', async () => {
526+
// Simple test that doesn't require complex mocking
527+
expect(
528+
executeBatchStrReplaces({
529+
deferredStrReplaces: [],
530+
toolCalls: [],
531+
toolResults: [],
532+
ws: {} as any,
533+
fileContext: mockFileContext,
534+
agentStepId: 'test-step',
535+
clientSessionId: 'test-session',
536+
userInputId: 'test-input',
537+
onResponseChunk: () => {},
538+
state: { messages: [] },
539+
userId: 'test-user',
540+
}),
541+
).resolves.toBeUndefined() // Should complete without throwing
542+
})
543+
})
544+
545+
it('should identify Benchify-supported file types correctly', () => {
546+
const testCases = [
547+
{ path: 'component.tsx', expected: true },
548+
{ path: 'utils.ts', expected: true },
549+
{ path: 'script.js', expected: true },
550+
{ path: 'styles.jsx', expected: true },
551+
{ path: 'README.md', expected: false },
552+
{ path: 'config.json', expected: false },
553+
{ path: 'styles.css', expected: false },
554+
{ path: 'index.html', expected: false },
555+
{ path: 'test.py', expected: false },
556+
]
557+
558+
for (const { path, expected } of testCases) {
559+
const result = benchifyCanFixLanguage(path)
560+
expect(result).toBe(expected)
561+
}
562+
})
563+
564+
it('should handle executeBatchStrReplaces with empty list', async () => {
565+
// Simple test that doesn't require complex mocking
566+
const result = await executeBatchStrReplaces({
567+
deferredStrReplaces: [],
568+
toolCalls: [],
569+
toolResults: [],
570+
ws: {} as any,
571+
fileContext: mockFileContext,
572+
agentStepId: 'test-step',
573+
clientSessionId: 'test-session',
574+
userInputId: 'test-input',
575+
onResponseChunk: () => {},
576+
state: { messages: [] },
577+
userId: 'test-user',
578+
})
579+
580+
// Should complete without throwing an error
581+
expect(result).toBeUndefined() // Function returns void
582+
})
583+
})

backend/src/process-str-replace.ts

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ export async function processStrReplace(
3535
let currentContent = initialContent
3636
let messages: string[] = []
3737
const lineEnding = currentContent.includes('\r\n') ? '\r\n' : '\n'
38+
let anyReplacementSuccessful = false
3839

3940
for (const { old: oldStr, new: newStr, allowMultiple } of replacements) {
4041
// Regular case: require oldStr for replacements
@@ -59,6 +60,7 @@ export async function processStrReplace(
5960

6061
if (match.success) {
6162
updatedOldStr = match.oldStr
63+
anyReplacementSuccessful = true
6264
} else {
6365
messages.push(match.error)
6466
updatedOldStr = null
@@ -72,15 +74,15 @@ export async function processStrReplace(
7274

7375
currentContent = currentContent.replaceAll('\n', lineEnding)
7476

75-
if (initialContent === currentContent) {
77+
// If no successful replacements occurred, return error
78+
if (!anyReplacementSuccessful) {
7679
logger.debug(
7780
{
7881
path,
7982
initialContent,
8083
},
81-
`processStrReplace: No change to ${path}`,
84+
`processStrReplace: No successful replacements for ${path}`,
8285
)
83-
messages.push('No change to the file.')
8486
return {
8587
tool: 'str_replace' as const,
8688
path,

0 commit comments

Comments
 (0)