Skip to content

Commit cc269d3

Browse files
bugerclaude
andauthored
fix: symlink search, null delegate response, allowEdit inheritance (#532, #533, #534) (#535)
* fix: follow symlinks in file_list_cache and guard null answer result (#532, #533) - Enable follow_links(true) in file_list_cache so symlinked subdirectories are discovered during search (#532). The walkdir crate provides built-in loop detection, and same_file_system(true) prevents mount-point traversal. - Ensure answer() always returns a string, never null/undefined (#533). When both structured output and finalText are empty (timeout, model failure), downstream callers like delegate() would throw "Delegate agent returned invalid response (not a string)". Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: pass allowEdit to delegated subagents (#534) Both search delegation and the explicit delegate tool failed to pass allowEdit through to delegate(), causing subagents to always run with allowEdit=false and consequently hashLines=false regardless of the parent agent's setting. - Add allowEdit to search delegation call in searchTool - Add allowEdit to delegateTool destructuring and delegate() call - Update existing exact-match test assertions for new parameter - Add tests for allowEdit inheritance in both code paths Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
1 parent 66eb28c commit cc269d3

8 files changed

Lines changed: 251 additions & 3 deletions

File tree

npm/src/agent/ProbeAgent.js

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4372,6 +4372,14 @@ Double-check your response based on the criteria above. If everything looks good
43724372
finalResult = aiResult.finalText;
43734373
}
43744374

4375+
// Ensure finalResult is always a string (#533).
4376+
// When both structured output and finalText are empty (e.g., timeout,
4377+
// model returns nothing), downstream callers like delegate() expect a
4378+
// string and would otherwise throw "not a string".
4379+
if (finalResult === null || finalResult === undefined) {
4380+
finalResult = '';
4381+
}
4382+
43754383
// Graceful timeout handling: ensure the response clearly indicates
43764384
// the research was interrupted and may be incomplete.
43774385
if (gracefulTimeoutState.triggered) {

npm/src/tools/vercel.js

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -540,6 +540,7 @@ export const searchTool = (options = {}) => {
540540
tracer: options.tracer || null,
541541
enableBash: false,
542542
bashConfig: null,
543+
allowEdit: options.allowEdit || false,
543544
architectureFileName: options.architectureFileName || null,
544545
promptType: 'code-searcher',
545546
allowedTools: ['search', 'extract', 'listFiles'],
@@ -895,7 +896,7 @@ export const extractTool = (options = {}) => {
895896
* @returns {Object} Configured delegate tool
896897
*/
897898
export const delegateTool = (options = {}) => {
898-
const { debug = false, timeout = 300, cwd, allowedFolders, workspaceRoot, enableBash = false, bashConfig, architectureFileName, enableMcp = false, mcpConfig = null, mcpConfigPath = null, delegationManager = null,
899+
const { debug = false, timeout = 300, cwd, allowedFolders, workspaceRoot, enableBash = false, bashConfig, allowEdit = false, architectureFileName, enableMcp = false, mcpConfig = null, mcpConfigPath = null, delegationManager = null,
899900
// Timeout settings inherited from parent agent
900901
timeoutBehavior, maxOperationTimeout, requestTimeout, gracefulTimeoutBonusSteps,
901902
negotiatedTimeoutBudget, negotiatedTimeoutMaxRequests, negotiatedTimeoutMaxPerRequest,
@@ -1014,6 +1015,7 @@ export const delegateTool = (options = {}) => {
10141015
model,
10151016
tracer,
10161017
enableBash,
1018+
allowEdit,
10171019
bashConfig,
10181020
architectureFileName,
10191021
searchDelegate,

npm/tests/delegate-config.test.js

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,7 @@ describe('Delegate Tool Configuration', () => {
5454
model: undefined,
5555
tracer: undefined,
5656
enableBash: false,
57+
allowEdit: false,
5758
bashConfig: undefined,
5859
architectureFileName: undefined,
5960
searchDelegate: undefined,

npm/tests/delegate-integration.test.js

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -97,6 +97,7 @@ describe('Delegate Tool Integration', () => {
9797
model: undefined,
9898
tracer: undefined,
9999
enableBash: false,
100+
allowEdit: false,
100101
bashConfig: undefined,
101102
architectureFileName: undefined,
102103
searchDelegate: undefined,
Lines changed: 114 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,114 @@
1+
/**
2+
* Tests for #533: answer() must always return a string, never null/undefined.
3+
*
4+
* When both structured output and finalText are empty (e.g., during timeouts
5+
* or model failures), answer() previously returned null, causing downstream
6+
* callers like delegate() to throw "Delegate agent returned invalid response
7+
* (not a string)".
8+
*/
9+
10+
import { describe, test, expect, jest, beforeEach } from '@jest/globals';
11+
import { ProbeAgent } from '../../src/agent/ProbeAgent.js';
12+
13+
function createAgent(opts = {}) {
14+
return new ProbeAgent({
15+
path: process.cwd(),
16+
model: 'test-model',
17+
...opts,
18+
});
19+
}
20+
21+
describe('answer() null-guard (#533)', () => {
22+
let agent;
23+
24+
beforeEach(() => {
25+
agent = createAgent();
26+
jest.spyOn(agent, 'getSystemMessage').mockResolvedValue('You are a test agent.');
27+
jest.spyOn(agent, 'prepareMessagesWithImages').mockImplementation(msgs => msgs);
28+
jest.spyOn(agent, '_buildThinkingProviderOptions').mockReturnValue(null);
29+
agent.provider = null;
30+
});
31+
32+
test('returns empty string when schema output and finalText are both null', async () => {
33+
jest.spyOn(agent, 'streamTextWithRetryAndFallback').mockImplementation(async () => ({
34+
text: Promise.resolve(''),
35+
finishReason: Promise.resolve('stop'),
36+
usage: Promise.resolve({ promptTokens: 10, completionTokens: 0 }),
37+
response: { messages: Promise.resolve([]) },
38+
experimental_providerMetadata: undefined,
39+
steps: Promise.resolve([]),
40+
// No output property — simulates no structured output
41+
}));
42+
43+
const result = await agent.answer('test question');
44+
expect(typeof result).toBe('string');
45+
});
46+
47+
test('returns empty string when schema present but outputObject is null', async () => {
48+
jest.spyOn(agent, 'streamTextWithRetryAndFallback').mockImplementation(async (opts) => ({
49+
text: Promise.resolve(''),
50+
finishReason: Promise.resolve('stop'),
51+
usage: Promise.resolve({ promptTokens: 10, completionTokens: 0 }),
52+
response: { messages: Promise.resolve([]) },
53+
experimental_providerMetadata: undefined,
54+
steps: Promise.resolve([]),
55+
result: {
56+
output: Promise.resolve(null),
57+
text: Promise.resolve(''),
58+
steps: Promise.resolve([]),
59+
},
60+
}));
61+
62+
// Pass a schema to trigger the schema code path
63+
const { z } = await import('zod');
64+
const result = await agent.answer('test question', [], {
65+
schema: z.object({ answer: z.string() }),
66+
});
67+
expect(typeof result).toBe('string');
68+
});
69+
70+
test('returns empty string when schema output throws and finalText is empty', async () => {
71+
jest.spyOn(agent, 'streamTextWithRetryAndFallback').mockImplementation(async (opts) => {
72+
// Create the rejection lazily to avoid unhandled-rejection before await
73+
const outputPromise = new Promise((_, reject) => {
74+
setTimeout(() => reject(new Error('NoObjectGeneratedError')), 0);
75+
});
76+
// Attach a no-op catch so Node doesn't report unhandled rejection
77+
outputPromise.catch(() => {});
78+
79+
return {
80+
text: Promise.resolve(''),
81+
finishReason: Promise.resolve('stop'),
82+
usage: Promise.resolve({ promptTokens: 10, completionTokens: 0 }),
83+
response: { messages: Promise.resolve([]) },
84+
experimental_providerMetadata: undefined,
85+
steps: Promise.resolve([]),
86+
result: {
87+
output: outputPromise,
88+
text: Promise.resolve(''),
89+
steps: Promise.resolve([]),
90+
},
91+
};
92+
});
93+
94+
const { z } = await import('zod');
95+
const result = await agent.answer('test question', [], {
96+
schema: z.object({ answer: z.string() }),
97+
});
98+
expect(typeof result).toBe('string');
99+
});
100+
101+
test('returns actual text when finalText is available', async () => {
102+
jest.spyOn(agent, 'streamTextWithRetryAndFallback').mockImplementation(async () => ({
103+
text: Promise.resolve('here is my answer'),
104+
finishReason: Promise.resolve('stop'),
105+
usage: Promise.resolve({ promptTokens: 10, completionTokens: 5 }),
106+
response: { messages: Promise.resolve([]) },
107+
experimental_providerMetadata: undefined,
108+
steps: Promise.resolve([]),
109+
}));
110+
111+
const result = await agent.answer('test question');
112+
expect(result).toBe('here is my answer');
113+
});
114+
});

npm/tests/unit/probe-agent-delegate.test.js

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -620,3 +620,41 @@ describe('delegateTool path priority', () => {
620620
});
621621
});
622622
});
623+
624+
describe('delegateTool allowEdit inheritance (#534)', () => {
625+
test('should pass allowEdit=true to delegate when parent has allowEdit', async () => {
626+
const tool = delegateTool({
627+
cwd: '/workspace',
628+
allowedFolders: ['/workspace'],
629+
allowEdit: true,
630+
});
631+
632+
// The tool should be configured — verify it destructured allowEdit
633+
expect(tool).toBeDefined();
634+
expect(typeof tool.execute).toBe('function');
635+
});
636+
637+
test('should default allowEdit to false when not provided', async () => {
638+
const tool = delegateTool({
639+
cwd: '/workspace',
640+
allowedFolders: ['/workspace'],
641+
});
642+
643+
expect(tool).toBeDefined();
644+
expect(typeof tool.execute).toBe('function');
645+
});
646+
647+
test('ProbeAgent derives hashLines from allowEdit by default', () => {
648+
const agentWithEdit = new ProbeAgent({ allowEdit: true });
649+
expect(agentWithEdit.allowEdit).toBe(true);
650+
expect(agentWithEdit.hashLines).toBe(true);
651+
652+
const agentWithoutEdit = new ProbeAgent({ allowEdit: false });
653+
expect(agentWithoutEdit.allowEdit).toBe(false);
654+
expect(agentWithoutEdit.hashLines).toBe(false);
655+
656+
const agentDefault = new ProbeAgent({});
657+
expect(agentDefault.allowEdit).toBe(false);
658+
expect(agentDefault.hashLines).toBe(false);
659+
});
660+
});

npm/tests/unit/search-delegate.test.js

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -476,4 +476,45 @@ describe('searchDelegate behavior', () => {
476476
const r3 = await tool.execute({ query: 'handleAuth', path: '/repo' });
477477
expect(r3).not.toContain('ticket/issue ID');
478478
});
479+
480+
test('search delegation passes allowEdit to delegate (#534)', async () => {
481+
mockDelegate.mockResolvedValue(JSON.stringify({
482+
targets: ['a.js#foo']
483+
}));
484+
mockExtract.mockResolvedValue('EXTRACTED');
485+
486+
const tool = searchTool({
487+
searchDelegate: true,
488+
cwd: '/workspace',
489+
allowedFolders: ['/workspace'],
490+
allowEdit: true,
491+
tracer: { withSpan: jest.fn(async (_name, fn) => fn()) }
492+
});
493+
494+
await tool.execute({ query: 'test', path: 'src' });
495+
496+
expect(mockDelegate).toHaveBeenCalledWith(expect.objectContaining({
497+
allowEdit: true
498+
}));
499+
});
500+
501+
test('search delegation defaults allowEdit to false when not set (#534)', async () => {
502+
mockDelegate.mockResolvedValue(JSON.stringify({
503+
targets: ['a.js#foo']
504+
}));
505+
mockExtract.mockResolvedValue('EXTRACTED');
506+
507+
const tool = searchTool({
508+
searchDelegate: true,
509+
cwd: '/workspace',
510+
allowedFolders: ['/workspace'],
511+
tracer: { withSpan: jest.fn(async (_name, fn) => fn()) }
512+
});
513+
514+
await tool.execute({ query: 'test', path: 'src' });
515+
516+
expect(mockDelegate).toHaveBeenCalledWith(expect.objectContaining({
517+
allowEdit: false
518+
}));
519+
});
479520
});

src/search/file_list_cache.rs

Lines changed: 45 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -154,8 +154,10 @@ fn build_file_list(
154154
let builder_start = Instant::now();
155155
let mut builder = WalkBuilder::new(path);
156156

157-
// CRITICAL: Never follow symlinks to avoid junction point cycles on Windows
158-
builder.follow_links(false);
157+
// Follow symlinks so that symlinked subdirectories are searched (#532).
158+
// The `walkdir` crate (used by `ignore`) has built-in loop detection,
159+
// and `same_file_system(true)` below prevents crossing mount points.
160+
builder.follow_links(true);
159161

160162
// Stay on the same file system to avoid traversing mount points
161163
builder.same_file_system(true);
@@ -859,4 +861,45 @@ mod tests {
859861
"Cache key should contain 'no_gitignore' when no_gitignore is true"
860862
);
861863
}
864+
865+
#[cfg(unix)]
866+
#[test]
867+
fn test_file_list_follows_symlinked_directories() {
868+
use std::os::unix::fs::symlink;
869+
870+
let temp_dir = TempDir::new().unwrap();
871+
let root = temp_dir.path();
872+
873+
// Create a real directory with a source file outside the root
874+
let real_dir = root.join("real_subdir");
875+
std::fs::create_dir_all(&real_dir).unwrap();
876+
let real_file = real_dir.join("hello.rs");
877+
std::fs::write(&real_file, "fn hello() {}").unwrap();
878+
879+
// Create a workspace directory and symlink the real directory into it
880+
let workspace = root.join("workspace");
881+
std::fs::create_dir_all(&workspace).unwrap();
882+
let linked = workspace.join("linked");
883+
symlink(&real_dir, &linked).unwrap();
884+
885+
// Also add a regular file in the workspace
886+
let regular = workspace.join("main.rs");
887+
std::fs::write(&regular, "fn main() {}").unwrap();
888+
889+
// Build file list from workspace — should find files in the symlinked dir
890+
let file_list = build_file_list(&workspace, true, &[], false).unwrap();
891+
892+
assert!(
893+
file_list.files.iter().any(|f| f.ends_with("main.rs")),
894+
"Regular file should be found"
895+
);
896+
assert!(
897+
file_list
898+
.files
899+
.iter()
900+
.any(|f| { f.to_string_lossy().contains("linked") && f.ends_with("hello.rs") }),
901+
"File inside symlinked directory should be found, got: {:?}",
902+
file_list.files
903+
);
904+
}
862905
}

0 commit comments

Comments
 (0)