From e62b3854cb91a4dc1ebbd165e1c3a219de4b2396 Mon Sep 17 00:00:00 2001
From: Mikael Hugo <mikkihugo@users.noreply.github.com>
Date: Thu, 30 Apr 2026 09:07:44 +0200
Subject: [PATCH] Prevent auto-commit after cancelled units

---
 src/resources/extensions/sf/auto/phases.ts    | 31 ++++++-
 .../cancelled-unit-no-autocommit.test.ts      | 80 +++++++++++++++++++
 2 files changed, 109 insertions(+), 2 deletions(-)
 create mode 100644 src/resources/extensions/sf/tests/cancelled-unit-no-autocommit.test.ts

diff --git a/src/resources/extensions/sf/auto/phases.ts b/src/resources/extensions/sf/auto/phases.ts
index eafbdf64a..10aa01d91 100644
--- a/src/resources/extensions/sf/auto/phases.ts
+++ b/src/resources/extensions/sf/auto/phases.ts
@@ -109,6 +109,28 @@ export function _resolveReportBasePath(
 	return s.originalBasePath || s.basePath;
 }
 
+function clearDeferredCommitAfterCancelledUnit(
+	s: AutoSession,
+	ctx: ExtensionContext,
+	unitType: string,
+	unitId: string,
+	reason: string,
+): void {
+	if (!s.stagedPendingCommit && !s.pendingCommitTaskContext) return;
+	s.stagedPendingCommit = false;
+	s.pendingCommitTaskContext = null;
+	debugLog("autoLoop", {
+		phase: "cancelled-unit-deferred-commit-cleared",
+		unitType,
+		unitId,
+		reason,
+	});
+	ctx.ui.notify(
+		`Cancelled ${unitType} ${unitId}; staged changes were preserved for recovery and not auto-committed.`,
+		"warning",
+	);
+}
+
 /**
  * Resolve the authoritative project base for dispatch guards.
  * Prior-milestone completion lives at the project root, even when the active
@@ -1874,6 +1896,13 @@ export async function runUnitPhase(
 	}
 
 	if (unitResult.status === "cancelled") {
+		clearDeferredCommitAfterCancelledUnit(
+			s,
+			ctx,
+			unitType,
+			unitId,
+			unitResult.errorContext?.message ?? "cancelled",
+		);
 		// Provider-error pause: pauseAuto already handled cleanup and scheduled
 		// recovery. Don't hard-stop — just break out of the loop (#2762).
 		if (unitResult.errorContext?.category === "provider") {
@@ -1910,7 +1939,6 @@ export async function runUnitPhase(
 				unitId,
 			});
 			await deps.pauseAuto(ctx, pi);
-			await deps.autoCommitUnit?.(s.basePath, unitType, unitId, ctx);
 			await emitCancelledUnitEnd(
 				ic,
 				unitType,
@@ -1931,7 +1959,6 @@ export async function runUnitPhase(
 				deps.buildSnapshotOpts(unitType, unitId),
 			);
 		}
-		await deps.autoCommitUnit?.(s.basePath, unitType, unitId, ctx);
 		await emitCancelledUnitEnd(
 			ic,
 			unitType,
diff --git a/src/resources/extensions/sf/tests/cancelled-unit-no-autocommit.test.ts b/src/resources/extensions/sf/tests/cancelled-unit-no-autocommit.test.ts
new file mode 100644
index 000000000..d1c1d871a
--- /dev/null
+++ b/src/resources/extensions/sf/tests/cancelled-unit-no-autocommit.test.ts
@@ -0,0 +1,80 @@
+/**
+ * Regression guard: cancelled units must not create normal auto-commits.
+ *
+ * A runaway-paused execute-task can return `status: "cancelled"` after the
+ * agent has staged work. That state is recovery territory, not a completed
+ * task. The cancelled branch must clear the deferred-commit flag and leave
+ * the operator/recovery path to decide what to do with the patch.
+ */
+
+import assert from "node:assert/strict";
+import { readFileSync } from "node:fs";
+import { join } from "node:path";
+import { test } from "node:test";
+
+const src = readFileSync(
+	join(import.meta.dirname, "..", "auto", "phases.ts"),
+	"utf-8",
+);
+
+function cancelledBranchBody(): string {
+	const marker = 'if (unitResult.status === "cancelled") {';
+	const start = src.indexOf(marker);
+	assert.notEqual(start, -1, "cancelled-unit branch must exist");
+	const nextMarker = "\n\t// \u2500\u2500 Immediate unit closeout";
+	const end = src.indexOf(nextMarker, start);
+	assert.notEqual(end, -1, "cancelled-unit branch end marker must exist");
+	return src.slice(start, end);
+}
+
+test("cancelled unit branch does not call autoCommitUnit", () => {
+	const body = cancelledBranchBody();
+	assert.equal(
+		body.includes("autoCommitUnit"),
+		false,
+		"cancelled units must not call normal autoCommitUnit",
+	);
+});
+
+test("cancelled unit branch clears deferred commit state before returning", () => {
+	const body = cancelledBranchBody();
+	const callIndex = body.indexOf("clearDeferredCommitAfterCancelledUnit(");
+	assert.ok(callIndex >= 0, "cancelled units must clear deferred commit state");
+	const providerReturnIndex = body.indexOf('reason: "provider-pause"');
+	assert.ok(
+		providerReturnIndex > callIndex,
+		"deferred commit state must be cleared before provider cancellation exits",
+	);
+	const timeoutReturnIndex = body.indexOf('reason: "session-timeout"');
+	assert.ok(
+		timeoutReturnIndex > callIndex,
+		"deferred commit state must be cleared before timeout cancellation exits",
+	);
+	const failedReturnIndex = body.indexOf('reason: "session-failed"');
+	assert.ok(
+		failedReturnIndex > callIndex,
+		"deferred commit state must be cleared before failed cancellation exits",
+	);
+});
+
+test("cancelled unit clear helper resets commit flags without committing", () => {
+	const helperName = "function clearDeferredCommitAfterCancelledUnit";
+	const start = src.indexOf(helperName);
+	assert.notEqual(start, -1, "clear helper must exist");
+	const end = src.indexOf("\n}\n", start + helperName.length);
+	assert.notEqual(end, -1, "clear helper must have a body");
+	const helper = src.slice(start, end);
+	assert.ok(
+		helper.includes("s.stagedPendingCommit = false"),
+		"helper must clear stagedPendingCommit",
+	);
+	assert.ok(
+		helper.includes("s.pendingCommitTaskContext = null"),
+		"helper must clear pendingCommitTaskContext",
+	);
+	assert.equal(
+		helper.includes("commitStaged(") || helper.includes("autoCommit"),
+		false,
+		"clear helper must not commit",
+	);
+});