diff --git a/src/actions/respond.ts b/src/actions/respond.ts index d1afed1..f85cf52 100644 --- a/src/actions/respond.ts +++ b/src/actions/respond.ts @@ -329,7 +329,9 @@ async function tryAutoResume( codexApprovalPolicy: session.codexApprovalPolicy, pendingPlanApproval: isPlanApproval ? false : session.pendingPlanApproval, planApprovalContext: session.planApprovalContext, - planDecisionVersion: session.planDecisionVersion, + planDecisionVersion: isPlanApproval + ? (session.actionablePlanDecisionVersion ?? session.planDecisionVersion) + 1 + : session.planDecisionVersion, actionablePlanDecisionVersion: isPlanApproval ? undefined : session.actionablePlanDecisionVersion, canonicalPlanPromptVersion: session.canonicalPlanPromptVersion, approvalPromptRequiredVersion: session.approvalPromptRequiredVersion, diff --git a/src/callback-handler.ts b/src/callback-handler.ts index ed68035..a4d08f6 100644 --- a/src/callback-handler.ts +++ b/src/callback-handler.ts @@ -14,6 +14,7 @@ import type { import type { PersistedSessionInfo, SessionActionKind, SessionActionToken } from "./types"; import { getRepoPolicyOption, validateRepoPolicyForPrAvailability } from "./repo-policy"; import { assessResumeCandidate } from "./session-resume"; +import { resolveCurrentPlanDecisionVersion, tokenMatchesAppliedPlanApproval } from "./plan-decision-state"; type InteractiveChannel = "telegram" | "discord"; type InteractiveCallbackContext = PluginInteractiveTelegramHandlerContext | PluginInteractiveDiscordHandlerContext; @@ -201,27 +202,6 @@ function planApprovalWasApplied(session: PlanDecisionTarget | undefined): boolea return session.approvalState === "approved" || !session.pendingPlanApproval; } -function latestDefinedVersion(...versions: Array): number | undefined { - let latest: number | undefined; - for (const version of versions) { - if (version == null) continue; - latest = latest == null ? version : Math.max(latest, version); - } - return latest; -} - -function resolveCurrentPlanDecisionVersion(session: PlanDecisionTarget): number | undefined { - if (session.actionablePlanDecisionVersion != null) return session.actionablePlanDecisionVersion; - - const deliveryVersion = latestDefinedVersion( - session.approvalPromptRequiredVersion, - session.approvalPromptVersion, - ); - if (deliveryVersion != null) return deliveryVersion; - - return session.canonicalPlanPromptVersion ?? session.planDecisionVersion; -} - function validatePlanDecisionToken( token: SessionActionToken, session: PlanDecisionTarget | undefined, @@ -696,6 +676,12 @@ export function createCallbackHandler( let sessionId = token.sessionId; let actionSession = sessionManager.resolve?.(sessionId) ?? sessionManager.getPersistedSession?.(sessionId); let actionSessionName = actionSession?.name ?? sessionId; + if (actionSession && tokenMatchesAppliedPlanApproval(token, actionSession)) { + sessionManager.consumePlanDecisionTokens?.(sessionId, token.planDecisionVersion!); + await clearPlanDecisionButtons(ctx, callbackAcknowledged); + await replyText(ctx, `✅ Plan v${token.planDecisionVersion} was already approved; resume is in progress or running.`); + return { handled: true }; + } let invalidPlanDecision = validatePlanDecisionToken(token, actionSession); logButtonDiagnostic("callback_plan_validation_completed", { channel: ctx.channel, @@ -829,6 +815,12 @@ export function createCallbackHandler( sessionId = latestToken.sessionId; actionSession = sessionManager.resolve?.(sessionId) ?? sessionManager.getPersistedSession?.(sessionId); actionSessionName = actionSession?.name ?? sessionId; + if (actionSession && tokenMatchesAppliedPlanApproval(latestToken, actionSession)) { + sessionManager.consumePlanDecisionTokens?.(sessionId, latestToken.planDecisionVersion!); + await clearPlanDecisionButtons(ctx, callbackAcknowledged); + await replyText(ctx, `✅ Plan v${latestToken.planDecisionVersion} was already approved; resume is in progress or running.`); + return { handled: true }; + } invalidPlanDecision = validatePlanDecisionToken(latestToken, actionSession); logButtonDiagnostic("callback_plan_validation_completed", { channel: ctx.channel, diff --git a/src/plan-decision-state.ts b/src/plan-decision-state.ts new file mode 100644 index 0000000..9d1b408 --- /dev/null +++ b/src/plan-decision-state.ts @@ -0,0 +1,155 @@ +import type { PersistedSessionInfo, PermissionMode, SessionConfig, SessionActionToken } from "./types"; + +export type PlanDecisionTarget = Pick< + PersistedSessionInfo, + | "approvalState" + | "pendingPlanApproval" + | "planApprovalContext" + | "planDecisionVersion" + | "actionablePlanDecisionVersion" + | "canonicalPlanPromptVersion" + | "approvalPromptRequiredVersion" + | "approvalPromptVersion" + | "approvalPromptStatus" + | "approvalPromptTransport" + | "approvalPromptMessageKind" + | "approvalPromptLastAttemptAt" + | "approvalPromptDeliveredAt" + | "approvalPromptFailedAt" +>; + +type PlanDecisionVersionTarget = Pick< + PlanDecisionTarget, + | "planDecisionVersion" + | "actionablePlanDecisionVersion" + | "canonicalPlanPromptVersion" + | "approvalPromptRequiredVersion" + | "approvalPromptVersion" +>; + +function latestDefinedVersion(...versions: Array): number | undefined { + let latest: number | undefined; + for (const version of versions) { + if (version == null) continue; + latest = latest == null ? version : Math.max(latest, version); + } + return latest; +} + +export function resolveCurrentPlanDecisionVersion(session: PlanDecisionVersionTarget): number | undefined { + if (session.actionablePlanDecisionVersion != null) return session.actionablePlanDecisionVersion; + const deliveryVersion = latestDefinedVersion( + session.approvalPromptRequiredVersion, + session.approvalPromptVersion, + ); + if (deliveryVersion != null) return deliveryVersion; + return session.canonicalPlanPromptVersion ?? session.planDecisionVersion; +} + +export function tokenMatchesAppliedPlanApproval( + token: Pick, + session: PlanDecisionVersionTarget & Pick, +): boolean { + return token.kind === "plan-approve" + && token.planDecisionVersion != null + && session.approvalState === "approved" + && !session.pendingPlanApproval + && session.planDecisionVersion === token.planDecisionVersion + 1; +} + +export type ResumedPlanState = { + permissionMode: PermissionMode; + approvalApplied: boolean; + decisionVersion?: number; + patch: Partial; +}; + +export function buildFailedPlanResumeRollbackState( + retryablePlan: PersistedSessionInfo, + postTerminal: PersistedSessionInfo | undefined, +): PersistedSessionInfo { + if (retryablePlan.worktreePath && postTerminal && !postTerminal.worktreePath) { + return { + ...retryablePlan, + worktreePath: undefined, + worktreeBranch: undefined, + worktreeState: postTerminal.worktreeState, + worktreeDisposition: postTerminal.worktreeDisposition, + worktreeLifecycle: postTerminal.worktreeLifecycle, + pendingWorktreeDecisionSince: postTerminal.pendingWorktreeDecisionSince, + lastWorktreeReminderAt: postTerminal.lastWorktreeReminderAt, + worktreeDecisionSnoozedUntil: postTerminal.worktreeDecisionSnoozedUntil, + worktreeMerged: postTerminal.worktreeMerged, + worktreeMergedAt: postTerminal.worktreeMergedAt, + worktreeDismissedAt: postTerminal.worktreeDismissedAt, + worktreeRemoteOutcome: postTerminal.worktreeRemoteOutcome, + }; + } + return retryablePlan; +} + +/** + * Carry a stable session's unresolved plan gate across runtime replacement. + * A bypassPermissions resume is an explicit approval only when an exact, + * actionable pending version exists; every other resume remains plan-gated. + */ +export function buildResumedPlanState( + session: PlanDecisionTarget, + requestedPermissionMode: PermissionMode, +): ResumedPlanState { + const decisionVersion = resolveCurrentPlanDecisionVersion(session); + if (!session.pendingPlanApproval) { + return { permissionMode: requestedPermissionMode, approvalApplied: false, patch: {} }; + } + + const common: Partial = { + planApprovalContext: session.planApprovalContext, + canonicalPlanPromptVersion: session.canonicalPlanPromptVersion, + approvalPromptRequiredVersion: session.approvalPromptRequiredVersion, + approvalPromptVersion: session.approvalPromptVersion, + approvalPromptStatus: session.approvalPromptStatus, + approvalPromptTransport: session.approvalPromptTransport, + approvalPromptMessageKind: session.approvalPromptMessageKind, + approvalPromptLastAttemptAt: session.approvalPromptLastAttemptAt, + approvalPromptDeliveredAt: session.approvalPromptDeliveredAt, + approvalPromptFailedAt: session.approvalPromptFailedAt, + }; + + if ( + requestedPermissionMode === "bypassPermissions" + && session.actionablePlanDecisionVersion != null + && session.actionablePlanDecisionVersion > 0 + && decisionVersion === session.actionablePlanDecisionVersion + && session.approvalState !== "rejected" + ) { + return { + permissionMode: "bypassPermissions", + approvalApplied: true, + decisionVersion, + patch: { + ...common, + pendingPlanApproval: false, + approvalState: "approved", + approvalExecutionState: "approved_then_implemented", + planModeApproved: true, + planDecisionVersion: decisionVersion + 1, + actionablePlanDecisionVersion: undefined, + }, + }; + } + + return { + // A stable-ID replacement must not turn a pending plan into executable + // default mode merely because the caller omitted the original mode. + permissionMode: "plan", + approvalApplied: false, + decisionVersion, + patch: { + ...common, + pendingPlanApproval: true, + approvalState: session.approvalState ?? "pending", + planDecisionVersion: decisionVersion ?? session.planDecisionVersion, + actionablePlanDecisionVersion: session.actionablePlanDecisionVersion, + }, + }; +} diff --git a/src/session-action-token-store.ts b/src/session-action-token-store.ts index 349eb9b..80e0c90 100644 --- a/src/session-action-token-store.ts +++ b/src/session-action-token-store.ts @@ -100,6 +100,23 @@ export class SessionActionTokenStore { return consumed; } + consumePlanDecisionTokens(sessionId: string, planDecisionVersion: number): SessionActionToken[] { + const consumed: SessionActionToken[] = []; + const consumedAt = Date.now(); + for (const token of this.tokens.values()) { + if ( + token.sessionId !== sessionId + || !isPlanDecisionKind(token.kind) + || token.planDecisionVersion !== planDecisionVersion + || token.consumedAt != null + ) continue; + token.consumedAt = consumedAt; + consumed.push(token); + } + if (consumed.length > 0) this.notifyChanged(); + return consumed; + } + deleteActionTokensForSession(sessionId: string): void { let changed = false; for (const [tokenId, token] of this.tokens) { diff --git a/src/session-interactions.ts b/src/session-interactions.ts index 9b1b9cf..099255e 100644 --- a/src/session-interactions.ts +++ b/src/session-interactions.ts @@ -60,6 +60,10 @@ export class SessionInteractionService { return this.actionTokens.consumeQuestionAnswerTokens(sessionId, requestId, questionId); } + consumePlanDecisionTokens(sessionId: string, planDecisionVersion: number): SessionActionToken[] { + return this.actionTokens.consumePlanDecisionTokens(sessionId, planDecisionVersion); + } + getActionToken(tokenId: string): SessionActionToken | undefined { return this.actionTokens.getActionToken(tokenId); } diff --git a/src/session-lifecycle-service.ts b/src/session-lifecycle-service.ts index 0f11e96..fd31196 100644 --- a/src/session-lifecycle-service.ts +++ b/src/session-lifecycle-service.ts @@ -284,6 +284,19 @@ export class SessionLifecycleService { return; } + // pendingPlanApproval is the deterministic gate. Lifecycle can lag behind + // approval/rejection during persistence recovery and must not suppress a + // real terminal worktree outcome after the decision has been resolved. + if (session.pendingPlanApproval) { + if (session.killReason === "idle-timeout" && session.pendingPlanApproval) { + this.emitIdleTimeoutPlanApproval(session); + } else { + await this.emitWaitingForInput(session); + } + this.deps.clearRetryTimersForSession(session.id); + return; + } + let worktreeResult: WorktreeStrategyResult = { notificationSent: false, worktreeRemoved: false, @@ -381,65 +394,8 @@ export class SessionLifecycleService { const costStr = `$${(session.costUsd ?? 0).toFixed(2)}`; const duration = session.duration; if (session.killReason === "idle-timeout") { - const planApprovalMode = session.pendingPlanApproval - ? this.deps.resolvePlanApprovalMode(session) - : undefined; if (session.pendingPlanApproval) { - const actionableVersion = session.actionablePlanDecisionVersion ?? session.planDecisionVersion; - const promptAlreadyProven = hasProvablePlanReviewPrompt(session, actionableVersion); - if (planApprovalMode === "delegate") { - this.deps.dispatchSessionNotification(session, { - label: "plan-approval-timeout", - idempotencyKey: `plan-approval-timeout:${session.id}:v${actionableVersion ?? "unknown"}:delegate`, - wakeMessage: [ - `[DELEGATED PLAN APPROVAL REMINDER] Plan review is still pending after the session hit idle timeout.`, - `Name: ${session.name} | ID: ${session.id}`, - this.deps.originThreadLine(session), - `The agent already produced a plan and is waiting for a delegated decision.`, - `Review privately first. Approve directly with agent_respond(..., approve=true, approval_rationale='...') if the plan is clearly within scope and low risk.`, - `Escalate only if needed via agent_request_plan_approval(summary='...').`, - `If you approve directly, follow up with a short user-facing explanation; the plugin's thumbs-up line is only the minimal approval acknowledgment.`, - `If a canonical approval prompt was already posted for this plan version, do not restate it in plain text.`, - ].join("\n"), - notifyUser: "never", - }); - this.deps.clearRetryTimersForSession(session.id); - return; - } - if (planApprovalMode === "ask" && promptAlreadyProven) { - this.deps.dispatchSessionNotification(session, { - label: "plan-approval-timeout", - idempotencyKey: `plan-approval-timeout:${session.id}:v${actionableVersion ?? "unknown"}:already-delivered`, - notifyUser: "never", - wakeMessage: [ - `[PLAN APPROVAL REMINDER] The user already has an actionable plan review prompt for this plan version.`, - `Name: ${session.name} | ID: ${session.id} | Plan v${actionableVersion ?? "?"}`, - this.deps.originThreadLine(session), - `Do NOT post another approval summary unless canonical delivery is known to be missing.`, - ].join("\n"), - }); - this.deps.clearRetryTimersForSession(session.id); - return; - } - this.deps.dispatchSessionNotification(session, { - label: "plan-approval-timeout", - idempotencyKey: `plan-approval-timeout:${session.id}:v${actionableVersion ?? "unknown"}:user-prompt`, - userMessage: [ - `📋 [${session.name}] Plan v${actionableVersion ?? "?"} still awaiting approval after idle timeout | ${costStr} | ${formatDuration(duration)}`, - ``, - `The agent already produced a plan and is waiting for your decision.`, - `Approve resumes the session and starts implementation.`, - `Revise resumes it in plan mode so it can update the plan first.`, - `Reject keeps the session stopped.`, - ].join("\n"), - notifyUser: "always", - buttons: planApprovalMode === "ask" && !promptAlreadyProven - ? this.deps.getPlanApprovalButtons(session.id, { - ...session, - planDecisionVersion: actionableVersion, - }) - : undefined, - }); + this.emitIdleTimeoutPlanApproval(session); this.deps.clearRetryTimersForSession(session.id); return; } @@ -458,6 +414,63 @@ export class SessionLifecycleService { this.deps.clearRetryTimersForSession(session.id); } + private emitIdleTimeoutPlanApproval(session: Session): void { + const planApprovalMode = this.deps.resolvePlanApprovalMode(session); + const actionableVersion = session.actionablePlanDecisionVersion ?? session.planDecisionVersion; + const promptAlreadyProven = hasProvablePlanReviewPrompt(session, actionableVersion); + if (planApprovalMode === "delegate") { + this.deps.dispatchSessionNotification(session, { + label: "plan-approval-timeout", + idempotencyKey: `plan-approval-timeout:${session.id}:v${actionableVersion ?? "unknown"}:delegate`, + wakeMessage: [ + `[DELEGATED PLAN APPROVAL REMINDER] Plan review is still pending after the session hit idle timeout.`, + `Name: ${session.name} | ID: ${session.id}`, + this.deps.originThreadLine(session), + `The agent already produced a plan and is waiting for a delegated decision.`, + `Review privately first. Approve directly with agent_respond(..., approve=true, approval_rationale='...') if the plan is clearly within scope and low risk.`, + `Escalate only if needed via agent_request_plan_approval(summary='...').`, + `If you approve directly, follow up with a short user-facing explanation; the plugin's thumbs-up line is only the minimal approval acknowledgment.`, + `If a canonical approval prompt was already posted for this plan version, do not restate it in plain text.`, + ].join("\n"), + notifyUser: "never", + }); + return; + } + if (planApprovalMode === "ask" && promptAlreadyProven) { + this.deps.dispatchSessionNotification(session, { + label: "plan-approval-timeout", + idempotencyKey: `plan-approval-timeout:${session.id}:v${actionableVersion ?? "unknown"}:already-delivered`, + notifyUser: "never", + wakeMessage: [ + `[PLAN APPROVAL REMINDER] The user already has an actionable plan review prompt for this plan version.`, + `Name: ${session.name} | ID: ${session.id} | Plan v${actionableVersion ?? "?"}`, + this.deps.originThreadLine(session), + `Do NOT post another approval summary unless canonical delivery is known to be missing.`, + ].join("\n"), + }); + return; + } + this.deps.dispatchSessionNotification(session, { + label: "plan-approval-timeout", + idempotencyKey: `plan-approval-timeout:${session.id}:v${actionableVersion ?? "unknown"}:user-prompt`, + userMessage: [ + `📋 [${session.name}] Plan v${actionableVersion ?? "?"} still awaiting approval after idle timeout | $${(session.costUsd ?? 0).toFixed(2)} | ${formatDuration(session.duration)}`, + ``, + `The agent already produced a plan and is waiting for your decision.`, + `Approve resumes the session and starts implementation.`, + `Revise resumes it in plan mode so it can update the plan first.`, + `Reject keeps the session stopped.`, + ].join("\n"), + notifyUser: "always", + buttons: planApprovalMode === "ask" && !promptAlreadyProven + ? this.deps.getPlanApprovalButtons(session.id, { + ...session, + planDecisionVersion: actionableVersion, + }) + : undefined, + }); + } + async emitWaitingForInput(session: Session): Promise { const pendingInputQuestions = session.pendingInputState?.questions; const activePendingInputQuestion = pendingInputQuestions?.[ diff --git a/src/session-manager.ts b/src/session-manager.ts index 5a3919d..cbdaf95 100644 --- a/src/session-manager.ts +++ b/src/session-manager.ts @@ -56,6 +56,7 @@ import { } from "./question-context-summary"; import { SessionRuntimeRegistry } from "./session-runtime-registry"; import { SessionRuntimeBootstrapService } from "./session-runtime-bootstrap-service"; +import { buildFailedPlanResumeRollbackState, buildResumedPlanState } from "./plan-decision-state"; import { SessionWorktreeMessageService } from "./session-worktree-message-service"; import { getSessionOutputPreview } from "./session-output-preview"; import { formatOriginRouteWakeBlock } from "./session-route"; @@ -251,6 +252,7 @@ export class SessionManager { private readonly runtimeBootstrap: SessionRuntimeBootstrapService; private readonly worktreeMessages: SessionWorktreeMessageService; private readonly maintenance: SessionMaintenanceService; + private readonly pendingPlanResumeClaims = new Map(); constructor( maxSessions: number = 20, @@ -443,9 +445,30 @@ export class SessionManager { }, markRunning: (session) => { store.markRunning(session); + if (session.approvalState === "approved" && session.planDecisionVersion > 0) { + interactions.consumePlanDecisionTokens(session.id, session.planDecisionVersion - 1); + } + manager.pendingPlanResumeClaims.delete(session.id); manager.onPersistedSessionChanged(store.getPersistedSession(session.id)); }, - handleTerminal: async (session) => manager.onSessionTerminal(session), + handleTerminal: async (session) => { + const retryablePlan = manager.pendingPlanResumeClaims.get(session.id); + try { + await manager.onSessionTerminal(session); + } finally { + // A failed approved resume must remain retryable even when terminal + // persistence, cleanup, or notification handling itself throws. + if (retryablePlan) { + const rollbackState = buildFailedPlanResumeRollbackState( + retryablePlan, + store.getPersistedSession(session.id), + ); + store.replacePersistedSession(rollbackState); + manager.pendingPlanResumeClaims.delete(session.id); + manager.onPersistedSessionChanged(rollbackState); + } + } + }, handleTurnEnd: (session, hadQuestion) => lifecycle.handleTurnEnd(session, hadQuestion), formatLaunchWorkdirLabel: (session) => manager.formatLaunchWorkdirLabel(session), notifySession: (session, text, label, idempotencyKey) => manager.notifySession(session, text, label, idempotencyKey), @@ -523,7 +546,23 @@ export class SessionManager { throw new Error(`Max sessions reached (${this.maxSessions}). Use agent_sessions to list active sessions and agent_kill to end one.`); } + let pendingPlanResumeClaim: PersistedSessionInfo | undefined; if (config.sessionIdOverride) { + const replacedPersisted = this.getPersistedSession(config.sessionIdOverride); + if (config.resumeSessionId && replacedPersisted?.pendingPlanApproval) { + const resumedPlanState = buildResumedPlanState( + replacedPersisted, + config.permissionMode ?? pluginConfig.permissionMode, + ); + config = { + ...config, + permissionMode: resumedPlanState.permissionMode, + ...resumedPlanState.patch, + }; + if (resumedPlanState.approvalApplied) { + pendingPlanResumeClaim = replacedPersisted; + } + } const existing = this.registry.get(config.sessionIdOverride); if (existing?.status === "starting" || existing?.status === "running") { throw new Error(`Cannot reuse session ID ${config.sessionIdOverride}: that session is still ${existing.status}.`); @@ -578,9 +617,17 @@ export class SessionManager { canUseTool, }, name); sessionIdRef = session.id; // bind late — canUseTool closure captures this ref + if (pendingPlanResumeClaim) { + this.pendingPlanResumeClaims.set(session.id, pendingPlanResumeClaim); + } this.registry.add(session); this.metrics.incrementLaunched(); - return this.runtimeBootstrap.initializeSession(session, preparedLaunch, config, options); + try { + return this.runtimeBootstrap.initializeSession(session, preparedLaunch, config, options); + } catch (err) { + this.pendingPlanResumeClaims.delete(session.id); + throw err; + } } /** Spawn a session and wait until it is truly running or fails before startup. */ @@ -1847,6 +1894,10 @@ export class SessionManager { return this.interactions.consumeQuestionAnswerTokens(sessionId, requestId, questionId); } + consumePlanDecisionTokens(sessionId: string, planDecisionVersion: number): SessionActionToken[] { + return this.interactions.consumePlanDecisionTokens(sessionId, planDecisionVersion); + } + dispose(): void { this.disposeMaintenance(); this.questions.dispose(); diff --git a/src/session-store.ts b/src/session-store.ts index 9af1174..6165c4d 100644 --- a/src/session-store.ts +++ b/src/session-store.ts @@ -197,6 +197,13 @@ export class SessionStore { private indexPersistedEntry(entry: PersistedSessionInfo): void { const storageKey = this.getEntryStorageKey(entry); + if (entry.sessionId) { + const replacedStorageKey = this.idIndex.get(entry.sessionId); + if (replacedStorageKey && replacedStorageKey !== storageKey) { + const replaced = this.persisted.get(replacedStorageKey); + if (replaced) this.removePersistedIndexes(replaced); + } + } this.persisted.set(storageKey, entry); if (entry.sessionId) this.idIndex.set(entry.sessionId, storageKey); if (entry.name) this.nameIndex.set(entry.name, storageKey); @@ -447,6 +454,13 @@ export class SessionStore { return this.queries.getPersistedSession(ref); } + replacePersistedSession(entry: PersistedSessionInfo): void { + const existing = entry.sessionId ? this.getPersistedSession(entry.sessionId) : undefined; + if (existing) this.removePersistedIndexes(existing); + this.indexPersistedEntry(entry); + this.saveIndex(); + } + /** List persisted sessions sorted by completion time (newest first). */ listPersistedSessions(): PersistedSessionInfo[] { return this.queries.listPersistedSessions(); diff --git a/src/session-worktree-controller.ts b/src/session-worktree-controller.ts index 24d10fa..ee640d2 100644 --- a/src/session-worktree-controller.ts +++ b/src/session-worktree-controller.ts @@ -1,6 +1,6 @@ import { existsSync } from "fs"; import type { PersistedSessionInfo } from "./types"; -import { getCommitsAheadCount, hasDirtyWorktreeEntries, isBranchAncestorOfBase, wouldMergeBeNoop } from "./worktree"; +import { getBranchName, getCommitsAheadCount, hasDirtyWorktreeEntries, isBranchAncestorOfBase, wouldMergeBeNoop } from "./worktree"; export type WorktreeCompletionState = | "no-change" @@ -17,6 +17,10 @@ export class SessionWorktreeController { branchName: string, baseBranch: string, ): WorktreeCompletionState { + // Never classify or clean a worktree whose checked-out branch does not + // match the session's persisted association. This can happen after a + // stale resume/recovery row points at a sibling replacement worktree. + if (getBranchName(worktreePath) !== branchName) return "has-commits"; const branchAheadCount = getCommitsAheadCount(repoDir, branchName, baseBranch); if (branchAheadCount === undefined) return "has-commits"; if (branchAheadCount === 0) { @@ -43,6 +47,8 @@ export class SessionWorktreeController { if (!existsSync(session.worktreePath)) return false; if (session.pendingWorktreeDecisionSince) return false; if (session.worktreeState === "pending_decision") return false; + if (session.pendingPlanApproval || session.resumable) return false; + if (session.worktreeBranch && getBranchName(session.worktreePath) !== session.worktreeBranch) return false; const resolvedAtIso = session.worktreeMergedAt diff --git a/src/tools/agent-launch.ts b/src/tools/agent-launch.ts index b85e7d3..2c9c35e 100644 --- a/src/tools/agent-launch.ts +++ b/src/tools/agent-launch.ts @@ -12,6 +12,7 @@ import { type AgentLaunchParams, } from "./agent-launch-resolution"; import { resolveSessionTaskLifecycle } from "../session-task-lifecycle"; +import { buildResumedPlanState } from "../plan-decision-state"; function errorMessage(err: unknown): string { return err instanceof Error ? err.message : String(err); @@ -211,6 +212,9 @@ export function makeAgentLaunchTool(ctx: OpenClawPluginToolContext) { ? resumeAssessment.stableSessionId : undefined) : undefined; + const resumedPlanState = resumeAssessment?.kind === "resume" && !params.fork_session && resumeTarget + ? buildResumedPlanState(resumeTarget, permissionMode) + : { permissionMode, approvalApplied: false, patch: {} }; if (launchWorktreeStrategy !== "off" && hasRequestRepoPolicyForLaunch(sessionManager)) { const policyCheck = sessionManager.checkRepoPolicyForLaunch(workdir, params.worktree_strategy); if (policyCheck.ok === false) { @@ -229,7 +233,7 @@ export function makeAgentLaunchTool(ctx: OpenClawPluginToolContext) { allowedTools: params.allowed_tools, resumeSessionId: resumeAssessment?.kind === "resume" ? resumeAssessment.resumeSessionId : resumeSessionId, resumedFromSessionName, - resumeWorktreeFrom: resolvedResumeId, + resumeWorktreeFrom: launchSessionIdOverride ?? params.resume_session_id ?? resolvedResumeId, sessionIdOverride: launchSessionIdOverride, clearedPersistedCodexResume, forkSession: resumeSessionId ? params.fork_session : false, @@ -261,12 +265,13 @@ export function makeAgentLaunchTool(ctx: OpenClawPluginToolContext) { resumedFromSessionName, // Worktree inheritance needs the original resolved session ref even when // backend resume state is intentionally cleared for a fresh launch. - resumeWorktreeFrom: resolvedResumeId, + resumeWorktreeFrom: launchSessionIdOverride ?? params.resume_session_id ?? resolvedResumeId, forkSession: resumeSessionId ? params.fork_session : false, multiTurn: true, - permissionMode, + permissionMode: resumedPlanState.permissionMode, planApproval, codexApprovalPolicy: harness === "codex" ? "never" : undefined, + ...resumedPlanState.patch, originChannel, originThreadId, originAgentId: ctx.agentId || undefined, @@ -278,13 +283,12 @@ export function makeAgentLaunchTool(ctx: OpenClawPluginToolContext) { worktreeBaseBranch: params.worktree_base_branch, worktreePrTargetRepo: params.worktree_pr_target_repo, }); - const launchText = hasFormatLaunchResult(sessionManager) ? sessionManager.formatLaunchResult({ prompt: params.prompt, workdir, harness, - permissionMode: permissionMode ?? pluginConfig.permissionMode, + permissionMode: resumedPlanState.permissionMode, planApproval, forceNewSession: params.force_new_session, resumeSessionId: params.resume_session_id, @@ -296,7 +300,7 @@ export function makeAgentLaunchTool(ctx: OpenClawPluginToolContext) { prompt: params.prompt, workdir, harness, - permissionMode: permissionMode ?? pluginConfig.permissionMode, + permissionMode: resumedPlanState.permissionMode, planApproval, resumeSessionId: params.resume_session_id, resumeSessionName: resumedFromSessionName, diff --git a/tests/agent-launch-tool.test.ts b/tests/agent-launch-tool.test.ts index c252f06..1c62405 100644 --- a/tests/agent-launch-tool.test.ts +++ b/tests/agent-launch-tool.test.ts @@ -337,7 +337,7 @@ describe("agent_launch tool defaults", () => { assert.equal(policyLaunchArgs?.harness, "codex"); assert.equal(policyLaunchArgs?.model, "gpt-5.6-sol"); assert.equal(policyLaunchArgs?.sessionIdOverride, "stable-session-1"); - assert.equal(policyLaunchArgs?.resumeWorktreeFrom, "resolved-stable-session-1"); + assert.equal(policyLaunchArgs?.resumeWorktreeFrom, "stable-session-1"); assert.equal(policyLaunchArgs?.originAgentId, "agent-main"); } finally { rmSync(workdir, { recursive: true, force: true }); @@ -481,6 +481,96 @@ describe("agent_launch tool defaults", () => { assert.match((result.content[0] as { text: string }).text, /ID: sess-stable/); }); + it("preserves a suspended pending plan when a stable-ID resume is not an approval", async () => { + let spawnConfig: Record | undefined; + const pendingPlan = { + sessionId: "sess-plan", + harnessSessionId: "thread-plan", + name: "pending-plan", + status: "killed", + lifecycle: "suspended", + killReason: "idle-timeout", + backendRef: { kind: "codex-app-server", conversationId: "thread-plan" }, + pendingPlanApproval: true, + approvalState: "pending", + planApprovalContext: "plan-mode", + planDecisionVersion: 4, + actionablePlanDecisionVersion: 4, + canonicalPlanPromptVersion: 4, + approvalPromptRequiredVersion: 4, + approvalPromptVersion: 4, + approvalPromptStatus: "delivered", + approvalPromptTransport: "direct-message", + approvalPromptMessageKind: "canonical_buttons", + }; + setSessionManager({ + resolve: () => undefined, + getPersistedSession: () => pendingPlan, + resolveHarnessSessionId: () => "thread-plan", + resolveBackendConversationId: () => "thread-plan", + spawn(config: Record) { + spawnConfig = config; + return { id: "sess-plan", name: "pending-plan", model: config.model }; + }, + } as any); + + const tool = makeAgentLaunchTool({ workspaceDir: "/tmp", oneShotCliRun: true }); + await tool.execute("tool-id", { + prompt: "Continue reviewing", + harness: "codex", + resume_session_id: "sess-plan", + permission_mode: "default", + }); + + assert.equal(spawnConfig?.permissionMode, "plan"); + assert.equal(spawnConfig?.pendingPlanApproval, true); + assert.equal(spawnConfig?.approvalState, "pending"); + assert.equal(spawnConfig?.planDecisionVersion, 4); + assert.equal(spawnConfig?.actionablePlanDecisionVersion, 4); + }); + + it("records exact approval state when bypass-resuming a suspended pending plan", async () => { + let spawnConfig: Record | undefined; + setSessionManager({ + resolve: () => undefined, + getPersistedSession: () => ({ + sessionId: "sess-plan", + harnessSessionId: "thread-plan", + name: "pending-plan", + status: "killed", + lifecycle: "suspended", + killReason: "idle-timeout", + backendRef: { kind: "codex-app-server", conversationId: "thread-plan" }, + pendingPlanApproval: true, + approvalState: "pending", + planApprovalContext: "plan-mode", + planDecisionVersion: 4, + actionablePlanDecisionVersion: 4, + }), + resolveHarnessSessionId: () => "thread-plan", + resolveBackendConversationId: () => "thread-plan", + spawn(config: Record) { + spawnConfig = config; + return { id: "sess-plan", name: "pending-plan", model: config.model }; + }, + } as any); + + const tool = makeAgentLaunchTool({ workspaceDir: "/tmp", oneShotCliRun: true }); + await tool.execute("tool-id", { + prompt: "The user approved Plan v4. Implement it.", + harness: "codex", + resume_session_id: "sess-plan", + permission_mode: "bypassPermissions", + }); + + assert.equal(spawnConfig?.permissionMode, "bypassPermissions"); + assert.equal(spawnConfig?.pendingPlanApproval, false); + assert.equal(spawnConfig?.approvalState, "approved"); + assert.equal(spawnConfig?.approvalExecutionState, "approved_then_implemented"); + assert.equal(spawnConfig?.planDecisionVersion, 5); + assert.equal(spawnConfig?.actionablePlanDecisionVersion, undefined); + }); + it("preserves the original session name when resuming without an explicit follow-up label", async () => { let spawnConfig: Record | undefined; diff --git a/tests/callback-handler.test.ts b/tests/callback-handler.test.ts index 2d2f66f..5aafe9c 100644 --- a/tests/callback-handler.test.ts +++ b/tests/callback-handler.test.ts @@ -1316,7 +1316,7 @@ describe("createCallbackHandler()", () => { assert.match(state.replies[0], /stream failed after approval/); }); - it("reports duplicate plan approval clicks as no longer awaiting approval", async () => { + it("reports duplicate plan approval clicks as an idempotent applied approval", async () => { let consumed = false; const token = { sessionId: "test-id", @@ -1331,6 +1331,7 @@ describe("createCallbackHandler()", () => { sendMessage: async () => { session.pendingPlanApproval = false; session.approvalState = "approved"; + session.planDecisionVersion = 2; session.actionablePlanDecisionVersion = undefined; }, switchPermissionMode: (mode: string) => { @@ -1364,7 +1365,79 @@ describe("createCallbackHandler()", () => { assert.equal(first.buttonMarkupEdits, 1); assert.equal(second.buttonMarkupEdits, 1); assert.deepEqual(first.replies, []); - assert.equal(second.replies[0], "⚠️ This plan is no longer awaiting approval."); + assert.equal(second.replies[0], "✅ Plan v1 was already approved; resume is in progress or running."); + }); + + it("approves and resumes an idle-timeout suspended persisted plan", async () => { + const token = { + id: "approve-suspended", + sessionId: "stable-plan", + kind: "plan-approve" as const, + planDecisionVersion: 6, + createdAt: Date.now(), + }; + const persisted = { + sessionId: "stable-plan", + harnessSessionId: "backend-plan", + backendRef: { kind: "codex-app-server", conversationId: "backend-plan" }, + name: "suspended-plan", + prompt: "Create the plan.", + workdir: "/tmp", + status: "killed", + lifecycle: "suspended", + runtimeState: "stopped", + killReason: "idle-timeout", + resumable: true, + pendingPlanApproval: true, + approvalState: "pending", + planApprovalContext: "plan-mode", + planDecisionVersion: 6, + actionablePlanDecisionVersion: 6, + currentPermissionMode: "plan", + requestedPermissionMode: "plan", + planApproval: "ask", + costUsd: 0, + route: { provider: "telegram", target: "1" }, + harness: "codex", + }; + let resumedConfig: any; + let active: any; + let consumed = 0; + setSessionManager({ + getActionToken: () => token, + getPersistedSession: () => persisted, + resolve: () => active, + spawnAndAwaitRunning: async (config: any) => { + resumedConfig = config; + active = createStubSession({ + id: "stable-plan", + name: "suspended-plan", + status: "running", + pendingPlanApproval: false, + approvalState: "approved", + planDecisionVersion: 7, + currentPermissionMode: "bypassPermissions", + startedAt: Date.now(), + }); + return active; + }, + consumeActionToken: () => { consumed++; return token; }, + notifySession: () => {}, + clearPlanDecisionTokens: () => {}, + } as any); + + const handler = createCallbackHandler(); + const state = createCtx("approve-suspended"); + const result = await handler.handler(state.ctx as any); + + assert.deepEqual(result, { handled: true }); + assert.equal(consumed, 1); + assert.equal(resumedConfig.sessionIdOverride, "stable-plan"); + assert.equal(resumedConfig.resumeSessionId, "backend-plan"); + assert.equal(resumedConfig.permissionMode, "bypassPermissions"); + assert.equal(resumedConfig.planDecisionVersion, 7); + assert.equal(resumedConfig.approvalState, "approved"); + assert.equal(state.buttonsCleared, 1); }); it("clears Telegram plan approval buttons and reports when the token is missing after successful approval", async () => { diff --git a/tests/plan-decision-state.test.ts b/tests/plan-decision-state.test.ts new file mode 100644 index 0000000..e39278c --- /dev/null +++ b/tests/plan-decision-state.test.ts @@ -0,0 +1,158 @@ +import { describe, it } from "node:test"; +import assert from "node:assert/strict"; +import { + buildFailedPlanResumeRollbackState, + buildResumedPlanState, + resolveCurrentPlanDecisionVersion, + tokenMatchesAppliedPlanApproval, +} from "../src/plan-decision-state"; + +const pending = { + pendingPlanApproval: true, + approvalState: "pending" as const, + planApprovalContext: "plan-mode" as const, + planDecisionVersion: 1, + actionablePlanDecisionVersion: 1, + canonicalPlanPromptVersion: 1, + approvalPromptRequiredVersion: 1, + approvalPromptVersion: 1, + approvalPromptStatus: "delivered" as const, + approvalPromptTransport: "direct-message" as const, + approvalPromptMessageKind: "canonical_buttons" as const, +}; + +describe("resumed plan decision state", () => { + it("preserves an unresolved plan gate across suspended runtime replacement", () => { + const result = buildResumedPlanState(pending, "default"); + assert.equal(result.permissionMode, "plan"); + assert.equal(result.approvalApplied, false); + assert.deepEqual(result.patch, { + pendingPlanApproval: true, + approvalState: "pending", + planApprovalContext: "plan-mode", + planDecisionVersion: 1, + actionablePlanDecisionVersion: 1, + canonicalPlanPromptVersion: 1, + approvalPromptRequiredVersion: 1, + approvalPromptVersion: 1, + approvalPromptStatus: "delivered", + approvalPromptTransport: "direct-message", + approvalPromptMessageKind: "canonical_buttons", + approvalPromptLastAttemptAt: undefined, + approvalPromptDeliveredAt: undefined, + approvalPromptFailedAt: undefined, + }); + }); + + it("applies an exact pending version on an explicit bypass resume", () => { + const result = buildResumedPlanState(pending, "bypassPermissions"); + assert.equal(result.approvalApplied, true); + assert.equal(result.decisionVersion, 1); + assert.equal(result.patch.pendingPlanApproval, false); + assert.equal(result.patch.approvalState, "approved"); + assert.equal(result.patch.planDecisionVersion, 2); + assert.equal(result.patch.actionablePlanDecisionVersion, undefined); + assert.equal(result.patch.approvalExecutionState, "approved_then_implemented"); + }); + + it("does not manufacture approval when no actionable version exists", () => { + const result = buildResumedPlanState({ + ...pending, + planDecisionVersion: 0, + actionablePlanDecisionVersion: undefined, + canonicalPlanPromptVersion: undefined, + approvalPromptRequiredVersion: undefined, + approvalPromptVersion: undefined, + }, "bypassPermissions"); + assert.equal(result.approvalApplied, false); + assert.equal(result.permissionMode, "plan"); + assert.equal(result.patch.pendingPlanApproval, true); + assert.equal(result.patch.actionablePlanDecisionVersion, undefined); + }); + + it("does not approve or recreate an actionable version after changes were requested", () => { + const result = buildResumedPlanState({ + ...pending, + approvalState: "changes_requested", + planDecisionVersion: 2, + actionablePlanDecisionVersion: undefined, + canonicalPlanPromptVersion: undefined, + approvalPromptRequiredVersion: undefined, + approvalPromptVersion: undefined, + }, "bypassPermissions"); + assert.equal(result.approvalApplied, false); + assert.equal(result.permissionMode, "plan"); + assert.equal(result.decisionVersion, 2); + assert.equal(result.patch.pendingPlanApproval, true); + assert.equal(result.patch.approvalState, "changes_requested"); + assert.equal(result.patch.actionablePlanDecisionVersion, undefined); + }); + + it("keeps terminal worktree cleanup authoritative when a failed approval resume is rolled back", () => { + const retryablePlan = { + ...pending, + sessionId: "stable-plan", + harnessSessionId: "backend-plan", + name: "stable-plan", + prompt: "implement", + workdir: "/repo", + worktreePath: "/repo/.worktrees/stable-plan", + worktreeBranch: "agent/stable-plan", + status: "killed" as const, + lifecycle: "suspended" as const, + costUsd: 0, + }; + const rollback = buildFailedPlanResumeRollbackState(retryablePlan, { + ...retryablePlan, + status: "failed", + lifecycle: "terminal", + pendingPlanApproval: false, + worktreePath: undefined, + worktreeBranch: undefined, + worktreeState: "none", + worktreeDisposition: "no-change-cleaned", + worktreeLifecycle: { + state: "no_change", + baseBranch: "main", + updatedAt: "2026-08-01T00:00:00.000Z", + }, + }); + assert.equal(rollback.pendingPlanApproval, true); + assert.equal(rollback.lifecycle, "suspended"); + assert.equal(rollback.worktreePath, undefined); + assert.equal(rollback.worktreeBranch, undefined); + assert.equal(rollback.worktreeState, "none"); + assert.equal(rollback.worktreeDisposition, "no-change-cleaned"); + assert.equal(rollback.worktreeLifecycle?.state, "no_change"); + }); + + it("prefers actionable and delivered versions over stale aggregate state", () => { + assert.equal(resolveCurrentPlanDecisionVersion({ + ...pending, + planDecisionVersion: 9, + actionablePlanDecisionVersion: 3, + }), 3); + assert.equal(resolveCurrentPlanDecisionVersion({ + ...pending, + actionablePlanDecisionVersion: undefined, + approvalPromptRequiredVersion: 4, + approvalPromptVersion: 5, + }), 5); + }); + + it("recognizes only the immediately applied approval version as idempotent", () => { + const approved = { ...pending, pendingPlanApproval: false, approvalState: "approved" as const, planDecisionVersion: 2 }; + assert.equal(tokenMatchesAppliedPlanApproval({ kind: "plan-approve", planDecisionVersion: 1 }, approved), true); + assert.equal(tokenMatchesAppliedPlanApproval({ kind: "plan-reject", planDecisionVersion: 1 }, approved), false); + assert.equal(tokenMatchesAppliedPlanApproval({ kind: "plan-approve", planDecisionVersion: 0 }, approved), false); + assert.equal(tokenMatchesAppliedPlanApproval({ kind: "plan-approve", planDecisionVersion: 1 }, { ...approved, planDecisionVersion: 3 }), false); + assert.equal(tokenMatchesAppliedPlanApproval({ kind: "plan-approve", planDecisionVersion: 1 }, { ...approved, approvalState: "rejected" }), false); + }); + + it("never bypasses a rejected plan even if corrupted state still marks it pending", () => { + const result = buildResumedPlanState({ ...pending, approvalState: "rejected" }, "bypassPermissions"); + assert.equal(result.permissionMode, "plan"); + assert.equal(result.approvalApplied, false); + assert.equal(result.patch.approvalState, "rejected"); + }); +}); diff --git a/tests/session-action-token-store.test.ts b/tests/session-action-token-store.test.ts index ab697f0..8e8f294 100644 --- a/tests/session-action-token-store.test.ts +++ b/tests/session-action-token-store.test.ts @@ -93,4 +93,23 @@ describe("SessionActionTokenStore", () => { assert.equal(store.getActionToken(newer.id)?.consumedAt, undefined); assert.deepEqual(store.listActiveActionTokens("question-answer").map((token) => token.id), [newer.id]); }); + + it("atomically consumes only one exact plan-decision version", () => { + const store = new SessionActionTokenStore(() => {}); + const approve = store.createActionToken("session-1", "plan-approve", { planDecisionVersion: 2 }); + const revise = store.createActionToken("session-1", "plan-request-changes", { planDecisionVersion: 2 }); + const reject = store.createActionToken("session-1", "plan-reject", { planDecisionVersion: 2 }); + const superseding = store.createActionToken("session-1", "plan-approve", { planDecisionVersion: 3 }); + const other = store.createActionToken("session-2", "plan-approve", { planDecisionVersion: 2 }); + + assert.deepEqual( + store.consumePlanDecisionTokens("session-1", 2).map((token) => token.id), + [approve.id, revise.id, reject.id], + ); + assert.ok(store.getActionToken(approve.id)?.consumedAt); + assert.ok(store.getActionToken(revise.id)?.consumedAt); + assert.ok(store.getActionToken(reject.id)?.consumedAt); + assert.equal(store.getActionToken(superseding.id)?.consumedAt, undefined); + assert.equal(store.getActionToken(other.id)?.consumedAt, undefined); + }); }); diff --git a/tests/session-manager.test.ts b/tests/session-manager.test.ts index aaf7ea9..7b79844 100644 --- a/tests/session-manager.test.ts +++ b/tests/session-manager.test.ts @@ -3379,6 +3379,98 @@ describe("SessionManager terminal wakes", () => { "terminal-completed:s-terminal-completed-at:completed:1700000004000:thread-terminal:2:unknown", ); }); + + it("does not resurrect cleaned worktree metadata when an approved resume fails before running", async () => { + const retryablePlan = { + sessionId: "s-failed-approved-resume", + harnessSessionId: "thread-failed-approved-resume", + backendRef: { kind: "codex-app-server" as const, conversationId: "thread-failed-approved-resume" }, + name: "failed-approved-resume", + prompt: "implement approved plan", + workdir: "/tmp/repo", + worktreePath: "/tmp/repo/.worktrees/failed-approved-resume", + worktreeBranch: "agent/failed-approved-resume", + status: "killed" as const, + lifecycle: "suspended" as const, + pendingPlanApproval: true, + approvalState: "pending" as const, + planDecisionVersion: 3, + actionablePlanDecisionVersion: 3, + costUsd: 0, + route: { + provider: "telegram", + accountId: "bot", + target: "12345", + threadId: "42", + sessionKey: "agent:main:telegram:group:12345:topic:42", + }, + }; + (sm as any).store.replacePersistedSession(retryablePlan); + (sm as any).pendingPlanResumeClaims.set(retryablePlan.sessionId, retryablePlan); + (sm as any).onSessionTerminal = async () => { + (sm as any).store.replacePersistedSession({ + ...retryablePlan, + status: "failed", + lifecycle: "terminal", + pendingPlanApproval: false, + approvalState: "approved", + worktreePath: undefined, + worktreeBranch: undefined, + }); + }; + + await (sm as any).runtimeBootstrap.deps.handleTerminal(fakeSession({ id: retryablePlan.sessionId })); + + const restored = sm.getPersistedSession(retryablePlan.sessionId); + assert.equal(restored?.status, "killed"); + assert.equal(restored?.lifecycle, "suspended"); + assert.equal(restored?.pendingPlanApproval, true); + assert.equal(restored?.worktreePath, undefined); + assert.equal(restored?.worktreeBranch, undefined); + assert.equal((sm as any).pendingPlanResumeClaims.has(retryablePlan.sessionId), false); + }); + + it("restores a retryable approved plan when terminal handling throws", async () => { + const retryablePlan = { + sessionId: "s-terminal-handler-failure", + harnessSessionId: "thread-terminal-handler-failure", + name: "terminal-handler-failure", + prompt: "implement approved plan", + workdir: "/tmp/repo", + status: "killed" as const, + lifecycle: "suspended" as const, + pendingPlanApproval: true, + approvalState: "pending" as const, + planDecisionVersion: 4, + actionablePlanDecisionVersion: 4, + costUsd: 0, + }; + (sm as any).store.replacePersistedSession(retryablePlan); + (sm as any).pendingPlanResumeClaims.set(retryablePlan.sessionId, retryablePlan); + (sm as any).onSessionTerminal = async () => { + (sm as any).store.replacePersistedSession({ + ...retryablePlan, + status: "failed", + lifecycle: "terminal", + pendingPlanApproval: false, + approvalState: "approved", + }); + throw new Error("terminal persistence failed"); + }; + + await assert.rejects( + (sm as any).runtimeBootstrap.deps.handleTerminal(fakeSession({ id: retryablePlan.sessionId })), + /terminal persistence failed/, + ); + + const restored = sm.getPersistedSession(retryablePlan.sessionId); + assert.equal(restored?.status, "killed"); + assert.equal(restored?.lifecycle, "suspended"); + assert.equal(restored?.pendingPlanApproval, true); + assert.equal(restored?.approvalState, "pending"); + assert.equal(restored?.actionablePlanDecisionVersion, 4); + assert.equal((sm as any).pendingPlanResumeClaims.has(retryablePlan.sessionId), false); + }); }); describe("SessionManager terminal wake behavior", () => { @@ -3665,6 +3757,11 @@ describe("SessionManager terminal wake behavior", () => { }); it("keeps timed-out pending plans in the plan-decision UX", async () => { + let worktreeStrategyCalls = 0; + (sm as any).worktreeStrategy.handleWorktreeStrategy = async () => { + worktreeStrategyCalls++; + return { notificationSent: true, worktreeRemoved: true }; + }; const s = fakeSession({ id: "s-plan-timeout", name: "spellcast-release-readiness-plan", @@ -3676,6 +3773,10 @@ describe("SessionManager terminal wake behavior", () => { isExplicitlyResumable: true, costUsd: 0, startedAt: Date.now() - 2_000, + originalWorkdir: "/tmp/repo", + worktreePath: "/tmp/repo/.worktrees/pending-plan", + worktreeBranch: "agent/pending-plan", + worktreeStrategy: "delegate", }); await (sm as any).onSessionTerminal(s); @@ -3692,6 +3793,34 @@ describe("SessionManager terminal wake behavior", () => { (request.buttons ?? []).map((row: Array<{ label: string }>) => row.map((button) => button.label)), [["Approve", "Revise", "Reject"]], ); + assert.equal(worktreeStrategyCalls, 0); + assert.equal(s.worktreePath, "/tmp/repo/.worktrees/pending-plan"); + }); + + it("does not let stale awaiting-plan lifecycle suppress a resolved terminal worktree outcome", async () => { + let worktreeStrategyCalls = 0; + (sm as any).worktreeStrategy.handleWorktreeStrategy = async () => { + worktreeStrategyCalls++; + return { notificationSent: true, worktreeRemoved: false }; + }; + const s = fakeSession({ + id: "s-stale-awaiting-plan", + name: "resolved-plan-implementation", + status: "completed", + lifecycle: "awaiting_plan_decision", + killReason: "done", + pendingPlanApproval: false, + approvalState: "approved", + originalWorkdir: "/tmp/repo", + worktreePath: "/tmp/repo/.worktrees/resolved-plan", + worktreeBranch: "agent/resolved-plan", + worktreeStrategy: "delegate", + }); + + await (sm as any).onSessionTerminal(s); + + assert.equal(worktreeStrategyCalls, 1); + assert.equal((sm as any).__dispatchCalls.length, 0); }); it("suppresses duplicate timed-out ask-mode summaries once a provable review prompt exists", async () => { diff --git a/tests/session-store.test.ts b/tests/session-store.test.ts index 4672194..9e9145b 100644 --- a/tests/session-store.test.ts +++ b/tests/session-store.test.ts @@ -34,6 +34,53 @@ function writeStore( }), "utf-8"); } +describe("SessionStore stable session replacement", () => { + it("keeps one canonical row and removes stale backend indexes", () => { + const dir = mkdtempSync(join(tmpdir(), "openclaw-store-stable-replacement-")); + try { + const indexPath = join(dir, "sessions.json"); + writeStore(indexPath, [ + { + sessionId: "stable-id", + harnessSessionId: "backend-old", + backendRef: { kind: "codex-app-server", conversationId: "backend-old" }, + name: "stable-session", + prompt: "plan", + workdir: "/tmp", + status: "killed", + lifecycle: "suspended", + pendingPlanApproval: true, + planDecisionVersion: 1, + costUsd: 0, + }, + { + sessionId: "stable-id", + harnessSessionId: "backend-new", + backendRef: { kind: "codex-app-server", conversationId: "backend-new" }, + name: "stable-session", + prompt: "implementation", + workdir: "/tmp", + status: "completed", + lifecycle: "terminal", + pendingPlanApproval: false, + approvalState: "approved", + planDecisionVersion: 2, + costUsd: 0, + }, + ]); + + const store = new SessionStore({ indexPath, env: {} }); + + assert.equal(store.listPersistedSessions().length, 1); + assert.equal(store.getPersistedSession("stable-id")?.harnessSessionId, "backend-new"); + assert.equal(store.getPersistedSession("backend-old"), undefined); + assert.equal(store.getPersistedSession("backend-new")?.planDecisionVersion, 2); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); +}); + describe("SessionStore getLatestPersistedByName", () => { let store: SessionStore; diff --git a/tests/session-worktree-controller.test.ts b/tests/session-worktree-controller.test.ts index a42a2c7..0345426 100644 --- a/tests/session-worktree-controller.test.ts +++ b/tests/session-worktree-controller.test.ts @@ -33,6 +33,28 @@ function installFakeGit(t: import("node:test").TestContext, scriptLines: string[ } describe("SessionWorktreeController.getCompletionState()", () => { + it("fails closed when persisted branch metadata points at a sibling worktree", () => { + const tempDir = mkdtempSync(join(tmpdir(), "session-worktree-controller-mismatch-")); + const repoDir = join(tempDir, "repo"); + const worktreePath = join(tempDir, "worktree"); + try { + mkdirSync(repoDir); + git(repoDir, "init", "-b", "main"); + git(repoDir, "config", "user.email", "test@example.com"); + git(repoDir, "config", "user.name", "Test User"); + writeFileSync(join(repoDir, "README.md"), "base\n"); + git(repoDir, "add", "README.md"); + git(repoDir, "commit", "-m", "base"); + git(repoDir, "worktree", "add", "-b", "agent/actual-branch", worktreePath, "main"); + const controller = new SessionWorktreeController(); + assert.equal( + controller.getCompletionState(repoDir, worktreePath, "agent/stale-branch", "main"), + "has-commits", + ); + } finally { + rmSync(tempDir, { recursive: true, force: true }); + } + }); it("classifies ahead branches with content already on base as released", () => { const tempDir = mkdtempSync(join(tmpdir(), "session-worktree-controller-released-")); const repoDir = join(tempDir, "repo"); @@ -150,6 +172,10 @@ describe("SessionWorktreeController.getCompletionState()", () => { "if [ \"$3\" = \"merge-base\" ]; then", " exit 1", "fi", + "if [ \"$3\" = \"rev-parse\" ]; then", + " echo feature", + " exit 0", + "fi", "if [ \"$3\" = \"status\" ]; then", " echo '?? dirty.txt'", " exit 0", @@ -212,3 +238,50 @@ describe("SessionWorktreeController.getCompletionState()", () => { }); }); + +describe("SessionWorktreeController.isResolvedWorktreeEligibleForCleanup()", () => { + it("preserves resumable and plan-gated worktrees", () => { + const tempDir = mkdtempSync(join(tmpdir(), "session-worktree-controller-retention-")); + const repoDir = join(tempDir, "repo"); + const worktreePath = join(tempDir, "worktree"); + try { + mkdirSync(repoDir); + git(repoDir, "init", "-b", "main"); + git(repoDir, "config", "user.email", "test@example.com"); + git(repoDir, "config", "user.name", "Test User"); + writeFileSync(join(repoDir, "README.md"), "base\n"); + git(repoDir, "add", "README.md"); + git(repoDir, "commit", "-m", "base"); + git(repoDir, "worktree", "add", "-b", "agent/retained", worktreePath, "main"); + const controller = new SessionWorktreeController(); + const base = { + sessionId: "retained", + harnessSessionId: "backend-retained", + name: "retained", + prompt: "p", + workdir: repoDir, + worktreePath, + worktreeBranch: "agent/retained", + status: "killed" as const, + lifecycle: "terminal" as const, + costUsd: 0, + completedAt: Date.now() - 60_000, + }; + assert.equal(controller.isResolvedWorktreeEligibleForCleanup({ ...base, resumable: true }, Date.now(), 1), false); + assert.equal(controller.isResolvedWorktreeEligibleForCleanup({ + ...base, + resumable: false, + lifecycle: "awaiting_plan_decision", + pendingPlanApproval: true, + }, Date.now(), 1), false); + assert.equal(controller.isResolvedWorktreeEligibleForCleanup({ + ...base, + resumable: false, + lifecycle: "awaiting_plan_decision", + pendingPlanApproval: false, + }, Date.now(), 1), true); + } finally { + rmSync(tempDir, { recursive: true, force: true }); + } + }); +});