fix(coding-agent): prevented asynchronous write races in plan reviews
- Ensures in-memory overlay edits are durably written to the plan file before proceeding with approval. - Avoids asynchronous write races by awaiting the final plan file serialization. - Aligns synthetic approved-plan prompts with reference-only expectations.
This commit is contained in:
@@ -3138,10 +3138,13 @@ export class InteractiveMode implements InteractiveModeContext {
|
||||
|
||||
if (choice === "Approve and execute" || choice === "Approve and compact context" || choice === keepContextLabel) {
|
||||
try {
|
||||
// Prefer in-overlay edits (already in memory) over a disk re-read; the
|
||||
// `onPlanEdited` write is fire-and-forget, so reading the file here could
|
||||
// race ahead of it.
|
||||
// Prefer in-overlay edits (already in memory) over a disk re-read. The
|
||||
// overlay mirrors edits as they happen, and approval awaits one final
|
||||
// write so the durable plan file and synthetic prompt carry the same text.
|
||||
const latestPlanContent = editedContent ?? (await this.#readPlanFile(planFilePath));
|
||||
if (editedContent !== undefined) {
|
||||
await Bun.write(this.#resolvePlanFilePath(planFilePath), editedContent);
|
||||
}
|
||||
if (!latestPlanContent) {
|
||||
this.showError(`Plan file not found at ${planFilePath}`);
|
||||
return;
|
||||
|
||||
@@ -10,7 +10,7 @@ const PLAN_FILE_PATH = "local://durable-plan.md";
|
||||
const PLAN_SENTINEL = "SENTINEL_HEADROOM_COMPRESSED_PLAN_CONTENT";
|
||||
|
||||
describe("approved plan execution prompts", () => {
|
||||
it("loads the approved plan from the durable local file instead of embedding content", () => {
|
||||
it("requires reading the durable plan file without inlining plan content", () => {
|
||||
const approved = prompt.render(planModeApprovedPrompt, {
|
||||
planContent: PLAN_SENTINEL,
|
||||
planFilePath: PLAN_FILE_PATH,
|
||||
@@ -26,6 +26,8 @@ describe("approved plan execution prompts", () => {
|
||||
|
||||
for (const rendered of [approved, reference, compact]) {
|
||||
expect(rendered).toContain(PLAN_FILE_PATH);
|
||||
}
|
||||
for (const rendered of [approved, reference, compact]) {
|
||||
expect(rendered).not.toContain(PLAN_SENTINEL);
|
||||
}
|
||||
expect(approved).toContain("MUST read `local://durable-plan.md`");
|
||||
|
||||
@@ -380,7 +380,14 @@ describe("InteractiveMode plan review rendering", () => {
|
||||
return "Approve and execute";
|
||||
});
|
||||
vi.spyOn(mode, "handleClearCommand").mockResolvedValue();
|
||||
const promptSpy = vi.spyOn(session, "prompt").mockResolvedValue(undefined as never);
|
||||
const promptSpy = vi.spyOn(session, "prompt").mockImplementation(async promptText => {
|
||||
if (typeof promptText === "string" && promptText.startsWith("Plan approved.")) {
|
||||
const persisted = await Bun.file(resolvedPlanPath).text();
|
||||
expect(persisted).toContain("edited body");
|
||||
expect(persisted).not.toContain("original body");
|
||||
}
|
||||
return undefined as never;
|
||||
});
|
||||
|
||||
await mode.handlePlanApproval({
|
||||
planFilePath,
|
||||
@@ -388,11 +395,11 @@ describe("InteractiveMode plan review rendering", () => {
|
||||
title: "PLAN",
|
||||
});
|
||||
|
||||
// The synthetic plan-approved prompt carries the in-overlay edit, not the
|
||||
// stale on-disk content (preferring editedContent avoids the write race).
|
||||
// The plan-approved prompt stays reference-only; approval must instead
|
||||
// await the durable file mirror before dispatch so read sees the edit.
|
||||
const call = promptSpy.mock.calls.find(isPlanApprovedCall);
|
||||
expect(call).toBeDefined();
|
||||
expect(call?.[0] as string).toContain("edited body");
|
||||
expect(call?.[0] as string).not.toContain("edited body");
|
||||
expect(call?.[0] as string).not.toContain("original body");
|
||||
// onPlanEdited mirrored the edit to the plan file.
|
||||
expect(await Bun.file(resolvedPlanPath).text()).toContain("edited body");
|
||||
|
||||
@@ -14,8 +14,8 @@ import {
|
||||
mergeTaskBranches,
|
||||
parseIsolationMode,
|
||||
} from "@oh-my-pi/pi-coding-agent/task/worktree";
|
||||
import * as jj from "@oh-my-pi/pi-coding-agent/utils/jj";
|
||||
import * as git from "@oh-my-pi/pi-coding-agent/utils/git";
|
||||
import * as jj from "@oh-my-pi/pi-coding-agent/utils/jj";
|
||||
import * as natives from "@oh-my-pi/pi-natives";
|
||||
import { removeWithRetries, setWorktreesDir } from "@oh-my-pi/pi-utils";
|
||||
|
||||
|
||||
@@ -3611,8 +3611,14 @@ export class TUI extends Container {
|
||||
repaintVirtualScrollInPlace: boolean;
|
||||
},
|
||||
): void {
|
||||
const { chunkTo, windowTop, prevWindowTop, prevHardwareCursorRow, forceWindowRewrite, repaintVirtualScrollInPlace } =
|
||||
options;
|
||||
const {
|
||||
chunkTo,
|
||||
windowTop,
|
||||
prevWindowTop,
|
||||
prevHardwareCursorRow,
|
||||
forceWindowRewrite,
|
||||
repaintVirtualScrollInPlace,
|
||||
} = options;
|
||||
const chunkFrom = this.#committedRows;
|
||||
const chunkLength = chunkTo - chunkFrom;
|
||||
const scroll = windowTop - prevWindowTop;
|
||||
|
||||
Reference in New Issue
Block a user