Skip to content

fix(core): handle nested plan files by resolving paths correctly #24854

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mahimashanware merged 1 commit into from
Apr 9, 2026
Merged

fix(core): handle nested plan files by resolving paths correctly #24854

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 37 additions & 0 deletions evals/plan_mode.eval.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -360,4 +360,41 @@ describe('plan_mode', () => {
assertModelHasOutput(result);
},
});

evalTest('ALWAYS_PASSES', {
name: 'should handle nested plan directories correctly',
approvalMode: ApprovalMode.PLAN,
params: {
settings,
},
prompt:
'Please create a new architectural plan in a nested folder called "architecture/frontend-v2.md" within the plans directory. The plan should contain the text "# Frontend V2 Plan". Do not ask for user approval, just create the plan.',
assert: async (rig, result) => {
await rig.waitForTelemetryReady();
const toolLogs = rig.readToolLogs();

const writeCalls = toolLogs.filter((log) =>
['write_file', 'replace'].includes(log.toolRequest.name),
);

const wroteToNestedPath = writeCalls.some((log) => {
try {
const args = JSON.parse(log.toolRequest.args);
return (
args.file_path &&
args.file_path.includes('architecture/frontend-v2.md')
);
} catch {
return false;
}
});

expect(
wroteToNestedPath,
'Expected model to successfully target the nested plan file path',
).toBe(true);

assertModelHasOutput(result);
},
});
});
4 changes: 2 additions & 2 deletions packages/core/src/tools/edit.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1318,8 +1318,8 @@ function doIt() {
vi.mocked(mockConfig.getPlansDir).mockReturnValue(plansDir);
vi.mocked(mockConfig.storage.getPlansDir).mockReturnValue(plansDir);

const filePath = path.join(rootDir, 'test-file.txt');
const planFilePath = path.join(plansDir, 'test-file.txt');
const filePath = 'test-file.txt';
const planFilePath = path.join(plansDir, filePath);
const initialContent = 'some initial content';
fs.writeFileSync(planFilePath, initialContent, 'utf8');

Expand Down
18 changes: 15 additions & 3 deletions packages/core/src/tools/edit.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ import { EDIT_DEFINITION } from './definitions/coreTools.js';
import { resolveToolDeclaration } from './definitions/resolver.js';
import { detectOmissionPlaceholders } from './omissionPlaceholderDetector.js';
import { discoverJitContext, appendJitContext } from './jit-context.js';
import { resolveAndValidatePlanPath } from '../utils/planUtils.js';

const ENABLE_FUZZY_MATCH_RECOVERY = true;
const FUZZY_MATCH_THRESHOLD = 0.1; // Allow up to 10% weighted difference
Expand Down Expand Up @@ -464,8 +465,10 @@ class EditToolInvocation
() => this.config.getApprovalMode(),
);
if (this.config.isPlanMode()) {
const safeFilename = path.basename(this.params.file_path);
this.resolvedPath = path.join(this.config.getPlansDir(), safeFilename);
this.resolvedPath = resolveAndValidatePlanPath(
this.params.file_path,
this.config.getPlansDir(),
);
} else if (!path.isAbsolute(this.params.file_path)) {
const result = correctPath(this.params.file_path, this.config);
if (result.success) {
Expand Down Expand Up @@ -1050,7 +1053,16 @@ export class EditTool
}

let resolvedPath: string;
if (!path.isAbsolute(params.file_path)) {
if (this.config.isPlanMode()) {
try {
resolvedPath = resolveAndValidatePlanPath(
params.file_path,
this.config.getPlansDir(),
);
} catch (err) {
return err instanceof Error ? err.message : String(err);
}
} else if (!path.isAbsolute(params.file_path)) {
const result = correctPath(params.file_path, this.config);
if (result.success) {
resolvedPath = result.correctedPath;
Expand Down
21 changes: 18 additions & 3 deletions packages/core/src/tools/write-file.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ import { debugLogger } from '../utils/debugLogger.js';
import { WRITE_FILE_DEFINITION } from './definitions/coreTools.js';
import { resolveToolDeclaration } from './definitions/resolver.js';
import { detectOmissionPlaceholders } from './omissionPlaceholderDetector.js';
import { resolveAndValidatePlanPath } from '../utils/planUtils.js';
import { isGemini3Model } from '../config/models.js';
import { discoverJitContext, appendJitContext } from './jit-context.js';

Expand Down Expand Up @@ -167,8 +168,10 @@ class WriteFileToolInvocation extends BaseToolInvocation<
);

if (this.config.isPlanMode()) {
const safeFilename = path.basename(this.params.file_path);
this.resolvedPath = path.join(this.config.getPlansDir(), safeFilename);
this.resolvedPath = resolveAndValidatePlanPath(
this.params.file_path,
this.config.getPlansDir(),
);
} else {
this.resolvedPath = path.resolve(
this.config.getTargetDir(),
Expand Down Expand Up @@ -493,7 +496,19 @@ export class WriteFileTool
return `Missing or empty "file_path"`;
}

const resolvedPath = path.resolve(this.config.getTargetDir(), filePath);
let resolvedPath: string;
if (this.config.isPlanMode()) {
try {
resolvedPath = resolveAndValidatePlanPath(
filePath,
this.config.getPlansDir(),
);
} catch (err) {
return err instanceof Error ? err.message : String(err);
}
} else {
resolvedPath = path.resolve(this.config.getTargetDir(), filePath);
}

const validationError = this.config.validatePathAccess(resolvedPath);
if (validationError) {
Expand Down
10 changes: 5 additions & 5 deletions packages/core/src/utils/planUtils.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,23 +31,23 @@ describe('planUtils', () => {

describe('validatePlanPath', () => {
it('should return null for a valid path within plans directory', async () => {
const planPath = path.join('plans', 'test.md');
const fullPath = path.join(tempRootDir, planPath);
const planPath = 'test.md';
const fullPath = path.join(plansDir, planPath);
fs.writeFileSync(fullPath, '# My Plan');

const result = await validatePlanPath(planPath, plansDir);
expect(result).toBeNull();
});

it('should return error for non-existent file', async () => {
const planPath = path.join('plans', 'ghost.md');
const planPath = 'ghost.md';
const result = await validatePlanPath(planPath, plansDir);
expect(result).toContain('Plan file does not exist');
});

it('should detect path traversal via symbolic links', async () => {
const maliciousPath = path.join('plans', 'malicious.md');
const fullMaliciousPath = path.join(tempRootDir, maliciousPath);
const maliciousPath = 'malicious.md';
const fullMaliciousPath = path.join(plansDir, maliciousPath);
const outsideFile = path.join(tempRootDir, 'outside.txt');
fs.writeFileSync(outsideFile, 'secret content');

Expand Down
53 changes: 40 additions & 13 deletions packages/core/src/utils/planUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,30 +23,57 @@ export const PlanErrorMessages = {
} as const;

/**
* Validates a plan file path for safety (traversal) and existence.
* @param planPath The untrusted path to the plan file.
* Resolves a plan file path and strictly validates it against the plans directory boundary.
* Useful for tools that need to write or read plans.
* @param planPath The untrusted file path provided by the model.
* @param plansDir The authorized project plans directory.
* @param targetDir The current working directory (project root).
* @returns An error message if validation fails, or null if successful.
* @returns The safely resolved path string.
* @throws Error if the path is empty, malicious, or escapes boundaries.
*/
export async function validatePlanPath(
export function resolveAndValidatePlanPath(
planPath: string,
plansDir: string,
): Promise<string | null> {
const safeFilename = path.basename(planPath);
const resolvedPath = path.join(plansDir, safeFilename);
): string {
const trimmedPath = planPath.trim();
if (!trimmedPath) {
throw new Error('Plan file path must be non-empty.');
}

const resolvedPath = path.resolve(plansDir, trimmedPath);
const realPath = resolveToRealPath(resolvedPath);
const realPlansDir = resolveToRealPath(plansDir);

if (!isSubpath(realPlansDir, realPath)) {
return PlanErrorMessages.PATH_ACCESS_DENIED(planPath, realPlansDir);
throw new Error(
`Security violation: plan path (${trimmedPath}) must be within the designated plans directory (${plansDir}).`,
);
}

if (!(await fileExists(resolvedPath))) {
return PlanErrorMessages.FILE_NOT_FOUND(planPath);
}
return resolvedPath;
}

return null;
/**
* Validates a plan file path for safety (traversal) and existence.
* @param planPath The untrusted path to the plan file.
* @param plansDir The authorized project plans directory.
* @returns An error message if validation fails, or null if successful.
*/
export async function validatePlanPath(
planPath: string,
plansDir: string,
): Promise<string | null> {
try {
const resolvedPath = resolveAndValidatePlanPath(planPath, plansDir);
if (!(await fileExists(resolvedPath))) {
return PlanErrorMessages.FILE_NOT_FOUND(planPath);
}
return null;
} catch {
return PlanErrorMessages.PATH_ACCESS_DENIED(
planPath,
resolveToRealPath(plansDir),
);
}
Comment on lines +71 to +76

@gemini-code-assist gemini-code-assist Bot Apr 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The catch block is overly broad, masking specific validation errors. Following repository guidelines, detailed errors should be logged for debugging instead of providing only a generic error message. Propagating the specific error message will improve diagnostics and user experience. Note that we assume configuration files are valid and avoid adding defensive code specifically for empty or whitespace-only strings.

  } catch (err) {
    console.error(err);
    if (err instanceof Error) {
      return err.message;
    }
    return PlanErrorMessages.PATH_ACCESS_DENIED(
      planPath,
      resolveToRealPath(plansDir),
    );
  }
References
  1. When catching exceptions, log the detailed error for debugging instead of providing only a generic error message.
  2. For internally managed configuration files, such as plan.toml, assume the configuration is valid and avoid adding defensive code to handle unlikely misconfigurations like empty or whitespace-only strings.

}

/**
Expand Down
Loading