Skip to content

Commit fd62c67

Browse files
brandonkachenjahooma
brandonkachen
and
jahooma
authored
[codex] Tolerate absolute file tool paths (#632)
Co-authored-by: James Grugett <jahooma@gmail.com>
1 parent 77ca87c commit fd62c67

8 files changed

Lines changed: 372 additions & 140 deletions

File tree

‎sdk/src/__tests__/change-file.test.ts‎

Lines changed: 99 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,37 @@ describe('changeFile', () => {
3636
)
3737
})
3838

39+
test('tolerates absolute paths inside the project for string replacements', async () => {
40+
const fs = createMockFs({
41+
files: {
42+
'/repo/src/file.ts': 'const value = 1\n',
43+
},
44+
})
45+
46+
const result = await changeFile({
47+
parameters: {
48+
type: 'patch',
49+
path: '/repo/src/file.ts',
50+
content: '@@ -1,1 +1,1 @@\n-const value = 1\n+const value = 2\n',
51+
},
52+
cwd: '/repo',
53+
fs,
54+
})
55+
56+
expect(result).toEqual([
57+
{
58+
type: 'json',
59+
value: {
60+
file: 'src/file.ts',
61+
message: 'String replace applied successfully.',
62+
},
63+
},
64+
])
65+
expect(await fs.readFile('/repo/src/file.ts', 'utf-8')).toBe(
66+
'const value = 2\n',
67+
)
68+
})
69+
3970
test('returns a simple success message for new file writes', async () => {
4071
const fs = createMockFs()
4172

@@ -63,6 +94,58 @@ describe('changeFile', () => {
6394
)
6495
})
6596

97+
test('tolerates absolute paths inside the project for file writes', async () => {
98+
const fs = createMockFs()
99+
100+
const result = await changeFile({
101+
parameters: {
102+
type: 'file',
103+
path: '/repo/src/file.ts',
104+
content: 'const value = 1\n',
105+
},
106+
cwd: '/repo',
107+
fs,
108+
})
109+
110+
expect(result).toEqual([
111+
{
112+
type: 'json',
113+
value: {
114+
file: 'src/file.ts',
115+
message: 'Created file successfully.',
116+
},
117+
},
118+
])
119+
expect(await fs.readFile('/repo/src/file.ts', 'utf-8')).toBe(
120+
'const value = 1\n',
121+
)
122+
})
123+
124+
test('accepts paths whose file names start with two dots inside the project', async () => {
125+
const fs = createMockFs()
126+
127+
const result = await changeFile({
128+
parameters: {
129+
type: 'file',
130+
path: '/repo/..config',
131+
content: 'value = true\n',
132+
},
133+
cwd: '/repo',
134+
fs,
135+
})
136+
137+
expect(result).toEqual([
138+
{
139+
type: 'json',
140+
value: {
141+
file: '..config',
142+
message: 'Created file successfully.',
143+
},
144+
},
145+
])
146+
expect(await fs.readFile('/repo/..config', 'utf-8')).toBe('value = true\n')
147+
})
148+
66149
test('returns a simple success message for overwritten file writes', async () => {
67150
const fs = createMockFs({
68151
files: {
@@ -93,4 +176,20 @@ describe('changeFile', () => {
93176
'const value = 2\n',
94177
)
95178
})
179+
180+
test('rejects absolute paths outside the project', async () => {
181+
const fs = createMockFs()
182+
183+
await expect(
184+
changeFile({
185+
parameters: {
186+
type: 'file',
187+
path: '/outside/file.ts',
188+
content: 'const value = 1\n',
189+
},
190+
cwd: '/repo',
191+
fs,
192+
}),
193+
).rejects.toThrow('file path is outside the project directory')
194+
})
96195
})

‎sdk/src/__tests__/path-utils.test.ts‎

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
import { describe, expect, test } from 'bun:test'
2+
3+
import {
4+
getProjectPathLookupKeys,
5+
resolveFilePathWithinProject,
6+
} from '../tools/path-utils'
7+
8+
describe('resolveFilePathWithinProject', () => {
9+
test('normalizes relative paths to full and project-relative paths', () => {
10+
expect(resolveFilePathWithinProject('/repo', 'src/file.ts')).toEqual({
11+
fullPath: '/repo/src/file.ts',
12+
relativePath: 'src/file.ts',
13+
})
14+
})
15+
16+
test('normalizes absolute paths inside the project', () => {
17+
expect(resolveFilePathWithinProject('/repo', '/repo/src/file.ts')).toEqual({
18+
fullPath: '/repo/src/file.ts',
19+
relativePath: 'src/file.ts',
20+
})
21+
})
22+
23+
test('allows file names that start with two dots inside the project', () => {
24+
expect(resolveFilePathWithinProject('/repo', '/repo/..config')).toEqual({
25+
fullPath: '/repo/..config',
26+
relativePath: '..config',
27+
})
28+
})
29+
30+
test('rejects paths outside the project', () => {
31+
expect(resolveFilePathWithinProject('/repo', '../outside.ts')).toBeNull()
32+
expect(resolveFilePathWithinProject('/repo', '/outside.ts')).toBeNull()
33+
expect(
34+
resolveFilePathWithinProject('/repo', '/repo-sibling/file.ts'),
35+
).toBeNull()
36+
})
37+
})
38+
39+
describe('getProjectPathLookupKeys', () => {
40+
test('returns the normalized relative key before the original absolute key', () => {
41+
expect(getProjectPathLookupKeys('/repo', '/repo/src/file.ts')).toEqual([
42+
'src/file.ts',
43+
'/repo/src/file.ts',
44+
])
45+
})
46+
47+
test('dedupes relative paths that are already normalized', () => {
48+
expect(getProjectPathLookupKeys('/repo', 'src/file.ts')).toEqual([
49+
'src/file.ts',
50+
])
51+
})
52+
53+
test('returns only the original key for paths outside the project', () => {
54+
expect(getProjectPathLookupKeys('/repo', '/outside.ts')).toEqual([
55+
'/outside.ts',
56+
])
57+
})
58+
})

‎sdk/src/__tests__/read-files.test.ts‎

Lines changed: 27 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -11,13 +11,11 @@ import {
1111
spyOn,
1212
} from 'bun:test'
1313

14-
1514
import { getFiles } from '../tools/read-files'
1615

1716
import type { CodebuffFileSystem } from '@codebuff/common/types/filesystem'
1817
import type { PathLike } from 'node:fs'
1918

20-
2119
// Helper to create a mock filesystem
2220
function createMockFs(config: {
2321
files?: Record<string, { content: string; size?: number }>
@@ -75,9 +73,10 @@ describe('getFiles', () => {
7573

7674
beforeEach(() => {
7775
// Default: no files are ignored
78-
isFileIgnoredSpy = spyOn(projectFileTree, 'isFileIgnored').mockResolvedValue(
79-
false,
80-
)
76+
isFileIgnoredSpy = spyOn(
77+
projectFileTree,
78+
'isFileIgnored',
79+
).mockResolvedValue(false)
8180
})
8281

8382
afterEach(() => {
@@ -320,9 +319,7 @@ describe('getFiles', () => {
320319

321320
test('should handle mix of ignored and non-ignored files', async () => {
322321
// First call returns false (not ignored), second returns true (ignored)
323-
isFileIgnoredSpy
324-
.mockResolvedValueOnce(false)
325-
.mockResolvedValueOnce(true)
322+
isFileIgnoredSpy.mockResolvedValueOnce(false).mockResolvedValueOnce(true)
326323

327324
const mockFs = createMockFs({
328325
files: {
@@ -393,7 +390,10 @@ describe('getFiles', () => {
393390
const mockFs = createMockFs({
394391
files: {},
395392
errors: {
396-
'/project/broken.ts': { code: 'EACCES', message: 'Permission denied' },
393+
'/project/broken.ts': {
394+
code: 'EACCES',
395+
message: 'Permission denied',
396+
},
397397
},
398398
})
399399

@@ -423,6 +423,24 @@ describe('getFiles', () => {
423423

424424
expect(result['src/index.ts']).toBe('content')
425425
})
426+
427+
test('should reject absolute paths in sibling directories with matching prefixes', async () => {
428+
const mockFs = createMockFs({
429+
files: {
430+
'/project-other/src/index.ts': { content: 'outside' },
431+
},
432+
})
433+
434+
const result = await getFiles({
435+
filePaths: ['/project-other/src/index.ts'],
436+
cwd: '/project',
437+
fs: mockFs,
438+
})
439+
440+
expect(result['/project-other/src/index.ts']).toBe(
441+
FILE_READ_STATUS.OUTSIDE_PROJECT,
442+
)
443+
})
426444
})
427445

428446
describe('fileFilter option', () => {

‎sdk/src/__tests__/run-file-filter.test.ts‎

Lines changed: 74 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,3 @@
1-
21
import * as mainPromptModule from '@codebuff/agent-runtime/main-prompt'
32
import { FILE_READ_STATUS } from '@codebuff/common/old-constants'
43
import * as projectFileTree from '@codebuff/common/project-file-tree'
@@ -91,9 +90,7 @@ describe('CodebuffClientOptions fileFilter', () => {
9190
let requestedFiles: Record<string, string | null> = {}
9291

9392
spyOn(mainPromptModule, 'callMainPrompt').mockImplementation(
94-
async (
95-
params: Parameters<typeof mainPromptModule.callMainPrompt>[0],
96-
) => {
93+
async (params: Parameters<typeof mainPromptModule.callMainPrompt>[0]) => {
9794
const { sendAction, promptId, requestFiles } = params
9895
const sessionState = getInitialSessionState(getStubProjectFileContext())
9996

@@ -177,9 +174,7 @@ describe('CodebuffClientOptions fileFilter', () => {
177174
let requestedFiles: Record<string, string | null> = {}
178175

179176
spyOn(mainPromptModule, 'callMainPrompt').mockImplementation(
180-
async (
181-
params: Parameters<typeof mainPromptModule.callMainPrompt>[0],
182-
) => {
177+
async (params: Parameters<typeof mainPromptModule.callMainPrompt>[0]) => {
183178
const { sendAction, promptId, requestFiles } = params
184179
const sessionState = getInitialSessionState(getStubProjectFileContext())
185180

@@ -259,9 +254,7 @@ describe('CodebuffClientOptions fileFilter', () => {
259254
let optionalFileResult: string | null = null
260255

261256
spyOn(mainPromptModule, 'callMainPrompt').mockImplementation(
262-
async (
263-
params: Parameters<typeof mainPromptModule.callMainPrompt>[0],
264-
) => {
257+
async (params: Parameters<typeof mainPromptModule.callMainPrompt>[0]) => {
265258
const { sendAction, promptId, requestOptionalFile } = params
266259
const sessionState = getInitialSessionState(getStubProjectFileContext())
267260

@@ -319,6 +312,75 @@ describe('CodebuffClientOptions fileFilter', () => {
319312
expect(optionalFileResult).toBeNull()
320313
})
321314

315+
it('should tolerate absolute requestOptionalFile paths inside cwd', async () => {
316+
spyOn(databaseModule, 'getUserInfoFromApiKey').mockResolvedValue({
317+
id: 'user-123',
318+
email: 'test@example.com',
319+
discord_id: null,
320+
stripe_customer_id: null,
321+
banned: false,
322+
created_at: new Date('2024-01-01T00:00:00Z'),
323+
})
324+
spyOn(databaseModule, 'fetchAgentFromDatabase').mockResolvedValue(null)
325+
spyOn(databaseModule, 'startAgentRun').mockResolvedValue('run-1')
326+
spyOn(databaseModule, 'finishAgentRun').mockResolvedValue(undefined)
327+
spyOn(databaseModule, 'addAgentStep').mockResolvedValue('step-1')
328+
spyOn(projectFileTree, 'isFileIgnored').mockResolvedValue(false)
329+
330+
const mockFs = createMockFs({
331+
files: {
332+
'/project/src/index.ts': { content: 'normal file content' },
333+
},
334+
})
335+
336+
const optionalFileResult: { current: string | null } = { current: null }
337+
338+
spyOn(mainPromptModule, 'callMainPrompt').mockImplementation(
339+
async (params: Parameters<typeof mainPromptModule.callMainPrompt>[0]) => {
340+
const { sendAction, promptId, requestOptionalFile } = params
341+
const sessionState = getInitialSessionState(getStubProjectFileContext())
342+
343+
optionalFileResult.current = await requestOptionalFile({
344+
filePath: '/project/src/index.ts',
345+
})
346+
347+
await sendAction({
348+
action: {
349+
type: 'prompt-response',
350+
promptId,
351+
sessionState,
352+
output: {
353+
type: 'lastMessage',
354+
value: [],
355+
},
356+
},
357+
})
358+
359+
return {
360+
sessionState,
361+
output: {
362+
type: 'lastMessage' as const,
363+
value: [],
364+
},
365+
}
366+
},
367+
)
368+
369+
const client = new CodebuffClient({
370+
apiKey: 'test-key',
371+
cwd: '/project',
372+
fsSource: mockFs,
373+
})
374+
375+
const result = await client.run({
376+
agent: 'base2',
377+
prompt: 'read optional file',
378+
})
379+
380+
expect(result.output.type).toBe('lastMessage')
381+
expect(optionalFileResult.current).toBe('normal file content')
382+
})
383+
322384
it('should allow all files when no fileFilter is provided', async () => {
323385
spyOn(databaseModule, 'getUserInfoFromApiKey').mockResolvedValue({
324386
id: 'user-123',
@@ -343,9 +405,7 @@ describe('CodebuffClientOptions fileFilter', () => {
343405
let requestedFiles: Record<string, string | null> = {}
344406

345407
spyOn(mainPromptModule, 'callMainPrompt').mockImplementation(
346-
async (
347-
params: Parameters<typeof mainPromptModule.callMainPrompt>[0],
348-
) => {
408+
async (params: Parameters<typeof mainPromptModule.callMainPrompt>[0]) => {
349409
const { sendAction, promptId, requestFiles } = params
350410
const sessionState = getInitialSessionState(getStubProjectFileContext())
351411

@@ -417,9 +477,7 @@ describe('CodebuffClientOptions fileFilter', () => {
417477
})
418478

419479
spyOn(mainPromptModule, 'callMainPrompt').mockImplementation(
420-
async (
421-
params: Parameters<typeof mainPromptModule.callMainPrompt>[0],
422-
) => {
480+
async (params: Parameters<typeof mainPromptModule.callMainPrompt>[0]) => {
423481
const { sendAction, promptId, requestFiles } = params
424482
const sessionState = getInitialSessionState(getStubProjectFileContext())
425483

0 commit comments

Comments
 (0)