From 985142fa50acb708e30cde4f9657bcfa4ca12cb7 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Mon, 25 May 2026 08:11:17 +0000 Subject: [PATCH] Fix duplicate failed_step interventions on mission run failure When a worker exhausts retries and the orchestrator run later fails, ensureTerminalFailedRunIntervention (#355) could open a second failed_step intervention for the same step. Worker paths store the orchestrator step id in metadata.stepId while terminal sync stores the mission step id, so dedup never matched. Unify matching across worker, terminal sync, and manual step-failure paths via failedStepInterventionsMatch, and route updateStep failures through the same dedup logic. Co-authored-by: Arul Sharma --- .../failedStepInterventionMatching.test.ts | 48 +++++++++++ .../failedStepInterventionMatching.ts | 47 +++++++++++ .../main/services/missions/missionService.ts | 74 +++++++++++------ .../aiOrchestratorService.test.ts | 79 +++++++++++++++++++ .../orchestrator/aiOrchestratorService.ts | 21 ++++- .../missions/missionInterventionRouting.ts | 9 ++- 6 files changed, 251 insertions(+), 27 deletions(-) create mode 100644 apps/desktop/src/main/services/missions/failedStepInterventionMatching.test.ts create mode 100644 apps/desktop/src/main/services/missions/failedStepInterventionMatching.ts diff --git a/apps/desktop/src/main/services/missions/failedStepInterventionMatching.test.ts b/apps/desktop/src/main/services/missions/failedStepInterventionMatching.test.ts new file mode 100644 index 000000000..45d6cce54 --- /dev/null +++ b/apps/desktop/src/main/services/missions/failedStepInterventionMatching.test.ts @@ -0,0 +1,48 @@ +import { describe, expect, it } from "vitest"; +import { failedStepInterventionsMatch } from "./failedStepInterventionMatching"; + +describe("failedStepInterventionsMatch", () => { + it("matches worker and terminal-sync metadata for the same failed step", () => { + const workerMetadata = { + runId: "run-1", + stepId: "orchestrator-step-1", + stepKey: "implement-api", + reasonCode: "retry_exhausted", + }; + expect( + failedStepInterventionsMatch(workerMetadata, { + missionStepId: "mission-step-1", + orchestratorStepId: "orchestrator-step-1", + stepKey: "implement-api", + runId: "run-1", + }), + ).toBe(true); + }); + + it("matches mission-step ids from terminal sync", () => { + expect( + failedStepInterventionsMatch( + { stepId: "mission-step-1", stepKey: "implement-api", runId: "run-1" }, + { missionStepId: "mission-step-1", orchestratorStepId: "orchestrator-step-1", runId: "run-1" }, + ), + ).toBe(true); + }); + + it("does not match different failed steps", () => { + expect( + failedStepInterventionsMatch( + { stepId: "mission-step-1", stepKey: "first-step", runId: "run-1" }, + { missionStepId: "mission-step-2", stepKey: "second-step", runId: "run-1" }, + ), + ).toBe(false); + }); + + it("does not match the same step key across different runs", () => { + expect( + failedStepInterventionsMatch( + { stepKey: "implement-api", runId: "run-1" }, + { stepKey: "implement-api", runId: "run-2" }, + ), + ).toBe(false); + }); +}); diff --git a/apps/desktop/src/main/services/missions/failedStepInterventionMatching.ts b/apps/desktop/src/main/services/missions/failedStepInterventionMatching.ts new file mode 100644 index 000000000..ce059ca82 --- /dev/null +++ b/apps/desktop/src/main/services/missions/failedStepInterventionMatching.ts @@ -0,0 +1,47 @@ +export type FailedStepInterventionTarget = { + missionStepId?: string | null; + orchestratorStepId?: string | null; + stepKey?: string | null; + runId?: string | null; +}; + +function stableString(value: unknown): string | null { + if (typeof value !== "string") return null; + const trimmed = value.trim(); + return trimmed.length > 0 ? trimmed : null; +} + +/** + * Match open failed_step interventions across worker, terminal-sync, and manual + * creation paths. `metadata.stepId` is not stable: worker interventions store + * the orchestrator step id there, while terminal sync stores the mission step id. + */ +export function failedStepInterventionsMatch( + existingMetadata: Record | null | undefined, + target: FailedStepInterventionTarget, +): boolean { + if (!existingMetadata) return false; + + const existingStepId = stableString(existingMetadata.stepId); + const existingOrchestratorStepId = stableString(existingMetadata.orchestratorStepId); + const existingStepKey = stableString(existingMetadata.stepKey); + const existingRunId = stableString(existingMetadata.runId); + + const missionStepId = stableString(target.missionStepId); + const orchestratorStepId = stableString(target.orchestratorStepId); + const stepKey = stableString(target.stepKey); + const runId = stableString(target.runId); + + if (missionStepId && existingStepId === missionStepId) return true; + + if (orchestratorStepId) { + if (existingStepId === orchestratorStepId) return true; + if (existingOrchestratorStepId === orchestratorStepId) return true; + } + + if (stepKey && existingStepKey === stepKey) { + if (!runId || !existingRunId || runId === existingRunId) return true; + } + + return false; +} diff --git a/apps/desktop/src/main/services/missions/missionService.ts b/apps/desktop/src/main/services/missions/missionService.ts index 9033ede99..a22733fb1 100644 --- a/apps/desktop/src/main/services/missions/missionService.ts +++ b/apps/desktop/src/main/services/missions/missionService.ts @@ -5,6 +5,7 @@ import { isValidResolutionKind, TERMINAL_MISSION_STATUSES, } from "../../../shared/types"; +import { failedStepInterventionsMatch } from "./failedStepInterventionMatching"; import type { AddMissionArtifactArgs, AddMissionInterventionArgs, @@ -4034,17 +4035,43 @@ export function createMissionService({ const stepKey = typeof stepMetadata?.stepKey === "string" && stepMetadata.stepKey.trim() ? stepMetadata.stepKey.trim() : null; - const intervention = insertIntervention({ - missionId, - interventionType: "failed_step", - title: `Step failed: ${step.title}`, - body: note ?? "A mission step was marked as failed and needs attention.", - requestedAction: "Review the failure and decide whether to continue, retry, or cancel.", - metadata: { - stepId, - ...(stepKey ? { stepKey } : {}), - }, - }); + const failedMetadata = { + stepId, + ...(stepKey ? { stepKey } : {}), + }; + const existingOpenRows = db.all( + `select id, mission_id, intervention_type, status, title, body, + requested_action, resolution_kind, resolution_note, lane_id, + created_at, updated_at, resolved_at, metadata_json + from mission_interventions + where mission_id = ? and project_id = ? and status = 'open' + and intervention_type = 'failed_step'`, + [missionId, projectId] + ); + const hasMatchingIntervention = existingOpenRows.some((row) => + failedStepInterventionsMatch(safeParseRecord(row.metadata_json), { + missionStepId: stepId, + stepKey, + }) + ); + let interventionId = + existingOpenRows.find((row) => + failedStepInterventionsMatch(safeParseRecord(row.metadata_json), { + missionStepId: stepId, + stepKey, + }) + )?.id ?? null; + if (!hasMatchingIntervention) { + const intervention = insertIntervention({ + missionId, + interventionType: "failed_step", + title: `Step failed: ${step.title}`, + body: note ?? "A mission step was marked as failed and needs attention.", + requestedAction: "Review the failure and decide whether to continue, retry, or cancel.", + metadata: failedMetadata, + }); + interventionId = intervention.id; + } db.run( ` @@ -4063,7 +4090,7 @@ export function createMissionService({ updatedAt, summary: "Mission paused for intervention after step failure.", payload: { - interventionId: intervention.id, + ...(interventionId ? { interventionId } : {}), stepId } }); @@ -4146,9 +4173,9 @@ export function createMissionService({ // For failed_step interventions, match by stepId in metadata. // For budget_limit_reached / provider_unreachable / unrecoverable_error, // match by interventionType alone (at most one open per type). - const meta = args.metadata ?? null; - const metaStepId = meta && typeof (meta as Record).stepId === "string" - ? ((meta as Record).stepId as string).trim() + const meta = isRecord(args.metadata) ? args.metadata : null; + const metaStepId = meta && typeof meta.stepId === "string" + ? meta.stepId.trim() : null; const existingOpenRows = db.all( @@ -4162,14 +4189,17 @@ export function createMissionService({ ); for (const row of existingOpenRows) { - if (args.interventionType === "failed_step" && metaStepId) { - // Match by stepId in metadata + if (args.interventionType === "failed_step" && meta) { const rowMeta = safeParseRecord(row.metadata_json); - const rowStepId = rowMeta && typeof rowMeta.stepId === "string" - ? rowMeta.stepId.trim() - : null; - if (rowStepId === metaStepId) { - // Return existing intervention — skip duplicate creation + if ( + failedStepInterventionsMatch(rowMeta, { + missionStepId: metaStepId, + orchestratorStepId: + typeof meta.orchestratorStepId === "string" ? meta.orchestratorStepId : null, + stepKey: typeof meta.stepKey === "string" ? meta.stepKey : null, + runId: typeof meta.runId === "string" ? meta.runId : null, + }) + ) { return toMissionIntervention(row); } } else if ( diff --git a/apps/desktop/src/main/services/orchestrator/aiOrchestratorService.test.ts b/apps/desktop/src/main/services/orchestrator/aiOrchestratorService.test.ts index 49075d3be..066f6af2c 100644 --- a/apps/desktop/src/main/services/orchestrator/aiOrchestratorService.test.ts +++ b/apps/desktop/src/main/services/orchestrator/aiOrchestratorService.test.ts @@ -7019,6 +7019,85 @@ describe("aiOrchestratorService", () => { } }); + it("does not duplicate worker failed-step interventions when a run fails", async () => { + const fixture = await createFixture(); + try { + const mission = fixture.missionService.create({ + prompt: "Avoid duplicate failed-step interventions on terminal sync.", + laneId: fixture.laneId, + plannedSteps: [ + { + index: 0, + title: "Implement API", + detail: "Worker failure", + kind: "implementation", + metadata: { stepType: "implementation", stepKey: "implement-api" }, + }, + ], + }); + const missionStep = fixture.missionService.get(mission.id)?.steps[0]; + if (!missionStep) throw new Error("Expected mission step"); + fixture.missionService.update({ missionId: mission.id, status: "in_progress" }); + + const started = fixture.orchestratorService.startRun({ + missionId: mission.id, + steps: [ + { + stepKey: "implement-api", + title: missionStep.title, + stepIndex: 0, + dependencyStepKeys: [], + executorKind: "manual", + missionStepId: missionStep.id, + metadata: { stepType: "implementation", stepKey: "implement-api" }, + }, + ], + }); + const runId = started.run.id; + const runStep = fixture.orchestratorService.getRunGraph({ runId }).steps[0]; + if (!runStep) throw new Error("Expected run step"); + + fixture.missionService.addIntervention({ + missionId: mission.id, + interventionType: "failed_step", + title: `Step failed: ${missionStep.title}`, + body: "Worker retries exhausted.", + requestedAction: "Review the failure.", + metadata: { + runId, + stepId: runStep.id, + stepKey: "implement-api", + reasonCode: "retry_exhausted", + }, + }); + + const failedAt = new Date().toISOString(); + fixture.db.run( + `update orchestrator_runs set status = 'failed', updated_at = ? where id = ?`, + [failedAt, runId], + ); + fixture.db.run( + `update orchestrator_steps set status = 'failed', completed_at = ?, updated_at = ? where run_id = ?`, + [failedAt, failedAt, runId], + ); + fixture.db.run( + `update mission_steps set status = 'failed', completed_at = ?, updated_at = ? where id = ?`, + [failedAt, failedAt, missionStep.id], + ); + + await fixture.aiOrchestratorService.syncMissionFromRun(runId, "run_failed"); + + const refreshed = fixture.missionService.get(mission.id); + const failedStepInterventions = refreshed?.interventions.filter((entry) => + entry.status === "open" && entry.interventionType === "failed_step" + ) ?? []; + expect(failedStepInterventions).toHaveLength(1); + expect(failedStepInterventions[0]?.metadata?.stepId).toBe(runStep.id); + } finally { + fixture.dispose(); + } + }); + it("opens terminal failed-step interventions for each matched failed mission step", async () => { const fixture = await createFixture(); try { diff --git a/apps/desktop/src/main/services/orchestrator/aiOrchestratorService.ts b/apps/desktop/src/main/services/orchestrator/aiOrchestratorService.ts index 86e74d836..775587490 100644 --- a/apps/desktop/src/main/services/orchestrator/aiOrchestratorService.ts +++ b/apps/desktop/src/main/services/orchestrator/aiOrchestratorService.ts @@ -109,6 +109,7 @@ import { isWorkerBootstrapNoiseLine } from "../../../shared/workerRuntimeNoise"; import type { Logger } from "../logging/logger"; import type { AdeDb } from "../state/kvDb"; import type { createMissionService } from "../missions/missionService"; +import { failedStepInterventionsMatch } from "../missions/failedStepInterventionMatching"; import type { createOrchestratorService } from "./orchestratorService"; import type { createProjectConfigService } from "../config/projectConfigService"; import type { createAiIntegrationService } from "../ai/aiIntegrationService"; @@ -7056,15 +7057,25 @@ Check all worker statuses and continue managing the mission from here. Read work if (!mission) return; const missionStepById = new Map(mission.steps.map((step) => [step.id, step])); + const readMissionStepKey = ( + missionStep: MissionDetail["steps"][number] | null, + ): string | null => { + if (!missionStep) return null; + const metadata = isRecord(missionStep.metadata) ? missionStep.metadata : null; + return stableString(metadata?.stepKey); + }; const hasMatchingOpenIntervention = ( failedRunStep: OrchestratorRunGraph["steps"][number] | null, failedMissionStep: MissionDetail["steps"][number] | null, ) => mission.interventions.some((entry) => { if (entry.status !== "open" || entry.interventionType !== "failed_step") return false; const metadata = isRecord(entry.metadata) ? entry.metadata : null; - if (!metadata) return false; - if (failedMissionStep?.id && metadata.stepId === failedMissionStep.id) return true; - return Boolean(!failedMissionStep && failedRunStep?.id && metadata.orchestratorStepId === failedRunStep.id); + return failedStepInterventionsMatch(metadata, { + missionStepId: failedMissionStep?.id ?? null, + orchestratorStepId: failedRunStep?.id ?? null, + stepKey: stableString(failedRunStep?.stepKey) ?? readMissionStepKey(failedMissionStep), + runId: graph.run.id, + }); }); const failedRunPair = graph.steps .filter((step) => step.status === "failed") @@ -7082,6 +7093,8 @@ Check all worker statuses and continue managing the mission from here. Read work const failedMissionStep = failedRunPair?.missionStep ?? fallbackMissionStep; if (!failedRunStep && !failedMissionStep) return; const failedStepTitle = failedMissionStep?.title ?? failedRunStep?.title ?? "Run step"; + const failedStepKey = + stableString(failedRunStep?.stepKey) ?? readMissionStepKey(failedMissionStep); missionService.addIntervention({ missionId: mission.id, @@ -7092,7 +7105,7 @@ Check all worker statuses and continue managing the mission from here. Read work metadata: { runId: graph.run.id, stepId: failedMissionStep?.id ?? null, - stepKey: failedRunStep?.stepKey ?? null, + stepKey: failedStepKey, orchestratorStepId: failedRunStep?.id ?? null, reasonCode: "terminal_run_failed_step", } diff --git a/apps/desktop/src/renderer/components/missions/missionInterventionRouting.ts b/apps/desktop/src/renderer/components/missions/missionInterventionRouting.ts index 7c8b9eefa..fe097ea94 100644 --- a/apps/desktop/src/renderer/components/missions/missionInterventionRouting.ts +++ b/apps/desktop/src/renderer/components/missions/missionInterventionRouting.ts @@ -45,6 +45,9 @@ export function routeMissionIntervention( const interventionStepId = typeof metadata.stepId === "string" && metadata.stepId.trim().length > 0 ? metadata.stepId.trim() : null; + const interventionOrchestratorStepId = typeof metadata.orchestratorStepId === "string" && metadata.orchestratorStepId.trim().length > 0 + ? metadata.orchestratorStepId.trim() + : null; const interventionStepKey = typeof metadata.stepKey === "string" && metadata.stepKey.trim().length > 0 ? metadata.stepKey.trim() : null; @@ -61,7 +64,11 @@ export function routeMissionIntervention( && interventionStepId && store.runGraph?.steps.some((step) => step.id === interventionStepId) ? interventionStepId - : null; + : sameRun + && interventionOrchestratorStepId + && store.runGraph?.steps.some((step) => step.id === interventionOrchestratorStepId) + ? interventionOrchestratorStepId + : null; if (reasonCode === "coordinator_unavailable" || reasonCode === "coordinator_recovery_failed") { store.setLogsFocusInterventionId(null);