feat: create parent directories when creating new files. (#552)
apply_patch doesn't create parent directories when creating a new file leading to confusion and flailing by the agent. This will create parent directories automatically when absent. --------- Co-authored-by: Thibault Sottiaux <tibo@openai.com>
This commit is contained in:
@@ -9,6 +9,7 @@ import { exec as rawExec } from "./sandbox/raw-exec.js";
|
|||||||
import { formatCommandForDisplay } from "../../format-command.js";
|
import { formatCommandForDisplay } from "../../format-command.js";
|
||||||
import fs from "fs";
|
import fs from "fs";
|
||||||
import os from "os";
|
import os from "os";
|
||||||
|
import path from "path";
|
||||||
import { parse } from "shell-quote";
|
import { parse } from "shell-quote";
|
||||||
import { resolvePathAgainstWorkdir } from "src/approvals.js";
|
import { resolvePathAgainstWorkdir } from "src/approvals.js";
|
||||||
|
|
||||||
@@ -63,7 +64,7 @@ export function exec(
|
|||||||
|
|
||||||
export function execApplyPatch(
|
export function execApplyPatch(
|
||||||
patchText: string,
|
patchText: string,
|
||||||
workdir: string | undefined,
|
workdir: string | undefined = undefined,
|
||||||
): ExecResult {
|
): ExecResult {
|
||||||
// This is a temporary measure to understand what are the common base commands
|
// This is a temporary measure to understand what are the common base commands
|
||||||
// until we start persisting and uploading rollouts
|
// until we start persisting and uploading rollouts
|
||||||
@@ -72,8 +73,20 @@ export function execApplyPatch(
|
|||||||
const result = process_patch(
|
const result = process_patch(
|
||||||
patchText,
|
patchText,
|
||||||
(p) => fs.readFileSync(resolvePathAgainstWorkdir(p, workdir), "utf8"),
|
(p) => fs.readFileSync(resolvePathAgainstWorkdir(p, workdir), "utf8"),
|
||||||
(p, c) =>
|
(p, c) => {
|
||||||
fs.writeFileSync(resolvePathAgainstWorkdir(p, workdir), c, "utf8"),
|
const resolvedPath = resolvePathAgainstWorkdir(p, workdir);
|
||||||
|
|
||||||
|
// Ensure the parent directory exists before writing the file. This
|
||||||
|
// mirrors the behaviour of the standalone apply_patch CLI (see
|
||||||
|
// write_file() in apply-patch.ts) and prevents errors when adding a
|
||||||
|
// new file in a not‑yet‑created sub‑directory.
|
||||||
|
const dir = path.dirname(resolvedPath);
|
||||||
|
if (dir !== ".") {
|
||||||
|
fs.mkdirSync(dir, { recursive: true });
|
||||||
|
}
|
||||||
|
|
||||||
|
fs.writeFileSync(resolvedPath, c, "utf8");
|
||||||
|
},
|
||||||
(p) => fs.unlinkSync(resolvePathAgainstWorkdir(p, workdir)),
|
(p) => fs.unlinkSync(resolvePathAgainstWorkdir(p, workdir)),
|
||||||
);
|
);
|
||||||
return {
|
return {
|
||||||
|
|||||||
44
codex-cli/tests/exec-apply-patch.test.ts
Normal file
44
codex-cli/tests/exec-apply-patch.test.ts
Normal file
@@ -0,0 +1,44 @@
|
|||||||
|
import { execApplyPatch } from "../src/utils/agent/exec.js";
|
||||||
|
import fs from "fs";
|
||||||
|
import os from "os";
|
||||||
|
import path from "path";
|
||||||
|
import { test, expect } from "vitest";
|
||||||
|
|
||||||
|
/**
|
||||||
|
* This test verifies that `execApplyPatch()` is able to add a new file whose
|
||||||
|
* parent directory does not yet exist. Prior to the fix, the call would throw
|
||||||
|
* because `fs.writeFileSync()` could not create intermediate directories. The
|
||||||
|
* test creates an isolated temporary directory to avoid polluting the project
|
||||||
|
* workspace.
|
||||||
|
*/
|
||||||
|
test("execApplyPatch creates missing directories when adding a file", () => {
|
||||||
|
const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "apply-patch-test-"));
|
||||||
|
|
||||||
|
// Ensure we start from a clean slate.
|
||||||
|
const nestedFileRel = path.join("foo", "bar", "baz.txt");
|
||||||
|
const nestedFileAbs = path.join(tmpDir, nestedFileRel);
|
||||||
|
expect(fs.existsSync(nestedFileAbs)).toBe(false);
|
||||||
|
|
||||||
|
const patch = `*** Begin Patch\n*** Add File: ${nestedFileRel}\n+hello new world\n*** End Patch`;
|
||||||
|
|
||||||
|
// Run execApplyPatch() with cwd switched to tmpDir so that the relative
|
||||||
|
// path in the patch is resolved inside the temporary location.
|
||||||
|
const prevCwd = process.cwd();
|
||||||
|
try {
|
||||||
|
process.chdir(tmpDir);
|
||||||
|
|
||||||
|
const result = execApplyPatch(patch);
|
||||||
|
expect(result.exitCode).toBe(0);
|
||||||
|
expect(result.stderr).toBe("");
|
||||||
|
} finally {
|
||||||
|
process.chdir(prevCwd);
|
||||||
|
}
|
||||||
|
|
||||||
|
// The file (and its parent directories) should have been created with the
|
||||||
|
// expected contents.
|
||||||
|
const fileContents = fs.readFileSync(nestedFileAbs, "utf8");
|
||||||
|
expect(fileContents).toBe("hello new world");
|
||||||
|
|
||||||
|
// Cleanup to keep tmpdir tidy.
|
||||||
|
fs.rmSync(tmpDir, { recursive: true, force: true });
|
||||||
|
});
|
||||||
Reference in New Issue
Block a user