diff --git a/doc/execution-semantics.md b/doc/execution-semantics.md index 6d34194ab0..9be7afd89a 100644 --- a/doc/execution-semantics.md +++ b/doc/execution-semantics.md @@ -152,6 +152,8 @@ Pre-dispatch configuration validation is a distinct gate that runs after ownersh A configuration-incomplete result is a gate outcome, not a runtime failure. It is one of the active gates that a checkout-time or dispatch-time check can surface instead of starting a run, and it leaves the issue in an explicit waiting state that names the missing binding. Surfacing the blocker keeps the issue healthy under the liveness contract while preventing a run that is guaranteed to fail once it cannot resolve its required secret/env bindings. A dispatched-then-failed run is the wrong shape for missing configuration: the missing binding is a known pre-dispatch condition, so the control plane must surface it as a configuration-incomplete blocker rather than letting the run start and then fail. +An unresolved workspace base ref is another configuration-incomplete condition. A `git_worktree` workspace bases a fresh worktree on a configured base ref. Paperclip first fetches a remote-only ref before dispatch: it maps an unqualified name (for example `fix/foo`) or a remote-tracking name (for example `origin/fix/foo`) to `origin/`, runs the authenticated fetch, and re-checks the commit. A ref that resolves lets work continue on the resolved commit. A ref that is still unresolvable after the fetch produces a configuration-incomplete blocker that names the requested ref, rather than a dispatched-then-failed run. Because the adapter never started, Paperclip queues no missing-comment retry. The recovery action dedupes by the canonical remote ref (`origin/`), not the operator spelling. Two equivalent spellings of one remote branch, for example `fix/foo` and `origin/fix/foo`, share one recovery identity, so a repeated failure reuses the active action and does not reset the attempt count or post a second notice. A different remote branch is a distinct blocker. Paperclip resolves the prior recovery action, creates a new action for the new ref, and notifies the operator with the new ref instead of overwriting the active action of the prior ref. + ## 6. Parent/Sub-Issue vs Blockers Paperclip uses two different relationships for different jobs. diff --git a/server/src/__tests__/heartbeat-process-recovery.test.ts b/server/src/__tests__/heartbeat-process-recovery.test.ts index dd33dbdb7f..25f031c945 100644 --- a/server/src/__tests__/heartbeat-process-recovery.test.ts +++ b/server/src/__tests__/heartbeat-process-recovery.test.ts @@ -3516,6 +3516,19 @@ describeEmbeddedPostgres("heartbeat orphaned process recovery", () => { errorCode: "workspace_validation_failed", }); expect(failedRun?.error).toContain("linked to a project workspace but has no project id"); + // The adapter process never started, so no agent could post an issue + // comment. The comment policy is not_applicable and no missing-comment + // retry is queued, which stops a pre-adapter setup failure from looping. + expect(failedRun?.processStartedAt).toBeNull(); + expect(failedRun?.issueCommentStatus).toBe("not_applicable"); + const missingCommentWakeups = await db + .select() + .from(agentWakeupRequests) + .where(and( + eq(agentWakeupRequests.companyId, companyId), + eq(agentWakeupRequests.reason, "missing_issue_comment"), + )); + expect(missingCommentWakeups).toHaveLength(0); expect(failedRun?.resultJson).toMatchObject({ workspaceValidation: { reason: "missing_project_id", diff --git a/server/src/__tests__/issue-recovery-actions.test.ts b/server/src/__tests__/issue-recovery-actions.test.ts index 0a2026b0c2..4fe95f14d4 100644 --- a/server/src/__tests__/issue-recovery-actions.test.ts +++ b/server/src/__tests__/issue-recovery-actions.test.ts @@ -331,6 +331,180 @@ describeEmbeddedPostgres("issue recovery actions", () => { }); }); + // Model the production payload: `requestedRef` keeps the operator spelling, + // and the fingerprint carries the canonical remote ref. Two equivalent + // spellings of one remote branch share `identityRef`, so they share one + // fingerprint. A different branch gets a different `identityRef`. + const makeUnresolvedBaseRefRun = (agentId: string, issueId: string) => + (requestedRef: string, identityRef: string) => + ({ + id: randomUUID(), + agentId, + status: "failed", + error: `Configured workspace base ref "${requestedRef}" did not resolve to a commit on origin after an authenticated fetch.`, + errorCode: "configuration_incomplete", + contextSnapshot: { issueId }, + livenessState: "needs_followup", + resultJson: { + configurationIncomplete: { + reason: "workspace_base_ref_unresolved", + requestedRef, + attemptedRefs: [identityRef], + fingerprint: `workspace_base_ref:${identityRef}`, + }, + }, + }) as const; + + it("bounds configuration-incomplete recovery by the unresolved base ref fingerprint", async () => { + const { coderId, sourceIssue } = await seedCompany(); + const enqueueWakeup = vi.fn(async () => null); + const recovery = recoveryService(db, { enqueueWakeup }); + const makeRun = makeUnresolvedBaseRefRun(coderId, sourceIssue.id); + + // Two reconciliations with the same unresolved ref reuse one active action. + await recovery.escalateStrandedAssignedIssue({ + issue: sourceIssue, + previousStatus: "in_progress", + latestRun: makeRun("fix/foo", "origin/fix/foo"), + recoveryCause: "configuration_incomplete", + }); + await recovery.escalateStrandedAssignedIssue({ + issue: sourceIssue, + previousStatus: "in_progress", + latestRun: makeRun("fix/foo", "origin/fix/foo"), + recoveryCause: "configuration_incomplete", + }); + + const actions = await db + .select() + .from(issueRecoveryActions) + .where(eq(issueRecoveryActions.sourceIssueId, sourceIssue.id)); + expect(actions).toHaveLength(1); + expect(actions[0]).toMatchObject({ + cause: "configuration_incomplete", + status: "active", + attemptCount: 2, + }); + // The fingerprint carries the canonical remote ref, so the same branch stays + // one action and a different branch would make a distinct fingerprint. + expect(actions[0]?.fingerprint).toBe( + `source_scoped_recovery:${sourceIssue.companyId}:${sourceIssue.id}:configuration_incomplete:workspace_base_ref:origin/fix/foo`, + ); + }); + + it("keeps equivalent spellings of one unresolved base ref under one recovery identity", async () => { + const { coderId, sourceIssue } = await seedCompany(); + const enqueueWakeup = vi.fn(async () => null); + const recovery = recoveryService(db, { enqueueWakeup }); + const makeRun = makeUnresolvedBaseRefRun(coderId, sourceIssue.id); + + // The operator retries the same remote branch under two spellings. Both map + // to the canonical `origin/fix/foo` identity, so recovery must not reset the + // attempt count or post a second notice. + await recovery.escalateStrandedAssignedIssue({ + issue: sourceIssue, + previousStatus: "in_progress", + latestRun: makeRun("fix/foo", "origin/fix/foo"), + recoveryCause: "configuration_incomplete", + }); + await recovery.escalateStrandedAssignedIssue({ + issue: sourceIssue, + previousStatus: "in_progress", + latestRun: makeRun("origin/fix/foo", "origin/fix/foo"), + recoveryCause: "configuration_incomplete", + }); + + const actions = await db + .select() + .from(issueRecoveryActions) + .where(eq(issueRecoveryActions.sourceIssueId, sourceIssue.id)); + // One identity, one active action, the attempt count advances. + expect(actions).toHaveLength(1); + expect(actions[0]).toMatchObject({ + cause: "configuration_incomplete", + status: "active", + attemptCount: 2, + }); + expect(actions[0]?.fingerprint).toBe( + `source_scoped_recovery:${sourceIssue.companyId}:${sourceIssue.id}:configuration_incomplete:workspace_base_ref:origin/fix/foo`, + ); + + // The operator gets one notice, bound to the one action. + const notices = await db + .select({ metadata: issueComments.metadata }) + .from(issueComments) + .where( + and( + eq(issueComments.issueId, sourceIssue.id), + eq(issueComments.authorType, "system"), + ), + ); + expect( + notices.filter((row) => + noticeMetadataReferencesRecoveryAction(row.metadata, actions[0]!.id), + ), + ).toHaveLength(1); + }); + + it("gives a distinct recovery identity and a new operator notice when the unresolved base ref changes", async () => { + const { coderId, sourceIssue } = await seedCompany(); + const enqueueWakeup = vi.fn(async () => null); + const recovery = recoveryService(db, { enqueueWakeup }); + const makeRun = makeUnresolvedBaseRefRun(coderId, sourceIssue.id); + + await recovery.escalateStrandedAssignedIssue({ + issue: sourceIssue, + previousStatus: "in_progress", + latestRun: makeRun("fix/foo", "origin/fix/foo"), + recoveryCause: "configuration_incomplete", + }); + await recovery.escalateStrandedAssignedIssue({ + issue: sourceIssue, + previousStatus: "in_progress", + latestRun: makeRun("fix/bar", "origin/fix/bar"), + recoveryCause: "configuration_incomplete", + }); + + const actions = await db + .select() + .from(issueRecoveryActions) + .where(eq(issueRecoveryActions.sourceIssueId, sourceIssue.id)); + // The prior ref keeps its own record and the new ref gets a fresh identity. + expect(actions).toHaveLength(2); + const priorAction = actions.find((row) => + row.fingerprint.endsWith("workspace_base_ref:origin/fix/foo"), + ); + const newAction = actions.find((row) => + row.fingerprint.endsWith("workspace_base_ref:origin/fix/bar"), + ); + expect(priorAction?.status).toBe("cancelled"); + expect(priorAction?.outcome).toBe("cancelled"); + expect(newAction?.status).toBe("active"); + expect(newAction?.attemptCount).toBe(1); + expect(newAction?.id).not.toBe(priorAction?.id); + + // The operator gets one notice per distinct ref, each bound to its action. + const systemComments = await db + .select({ metadata: issueComments.metadata }) + .from(issueComments) + .where( + and( + eq(issueComments.issueId, sourceIssue.id), + eq(issueComments.authorType, "system"), + ), + ); + expect( + systemComments.some((row) => + noticeMetadataReferencesRecoveryAction(row.metadata, priorAction!.id), + ), + ).toBe(true); + expect( + systemComments.some((row) => + noticeMetadataReferencesRecoveryAction(row.metadata, newAction!.id), + ), + ).toBe(true); + }); + it.each([ ["process_lost", undefined, "coder"], ["adapter_failed", "successful_run_missing_state", "coder"], diff --git a/server/src/__tests__/issue-update-comment-wakeup-routes.test.ts b/server/src/__tests__/issue-update-comment-wakeup-routes.test.ts index b8fcd6d1aa..d0846cb2b3 100644 --- a/server/src/__tests__/issue-update-comment-wakeup-routes.test.ts +++ b/server/src/__tests__/issue-update-comment-wakeup-routes.test.ts @@ -262,7 +262,10 @@ describe("issue update comment wakeups", () => { }); expect(res.status).toBe(200); - expect(mockHeartbeatService.wakeup).toHaveBeenCalledTimes(1); + // The route dispatches the wake after it sends the response, so wait for + // the fire-and-forget dispatch to settle. This keeps the wake inside this + // test and stops it from leaking into the next test as an extra call. + await vi.waitFor(() => expect(mockHeartbeatService.wakeup).toHaveBeenCalledTimes(1)); expect(mockHeartbeatService.wakeup).toHaveBeenCalledWith( ASSIGNEE_AGENT_ID, expect.objectContaining({ diff --git a/server/src/__tests__/workspace-runtime.test.ts b/server/src/__tests__/workspace-runtime.test.ts index c65f26c4d6..adf2db7574 100644 --- a/server/src/__tests__/workspace-runtime.test.ts +++ b/server/src/__tests__/workspace-runtime.test.ts @@ -36,6 +36,7 @@ import { realizeExecutionWorkspace, refreshRemoteTrackingBaseRef, releaseRuntimeServicesForRun, + UnresolvedWorkspaceBaseRefError, resetRuntimeServicesForTests, resolveRuntimeProvisionCommand, resolveWorkspaceRuntimeReadinessTimeoutSec, @@ -251,6 +252,26 @@ async function advanceRemoteMaster(sourceRepo: string, remotePath: string, fileN return readGit(sourceRepo, ["rev-parse", "master"]); } +// Push a branch to the bare remote without leaving a local ref or a +// remote-tracking ref in `repoRoot`. This reproduces a remote-only feature +// branch: the clone was made before the push, so `repoRoot` learns the branch +// only after an authenticated fetch of `origin/`. +async function pushRemoteOnlyBranch( + sourceRepo: string, + remotePath: string, + branch: string, + fileName: string, +) { + await runGit(sourceRepo, ["checkout", "-B", branch]); + await fs.writeFile(path.join(sourceRepo, fileName), `${fileName}\n`, "utf8"); + await runGit(sourceRepo, ["add", fileName]); + await runGit(sourceRepo, ["commit", "-m", `Add ${fileName}`]); + await runGit(sourceRepo, ["push", remotePath, branch]); + const sha = await readGit(sourceRepo, ["rev-parse", branch]); + await runGit(sourceRepo, ["checkout", "master"]); + return sha; +} + function realizeWorktreeForTest(repoRoot: string, repoRef: string | null) { return realizeExecutionWorkspace({ base: { @@ -1018,6 +1039,111 @@ describe("realizeExecutionWorkspace", () => { ]); }); + it("bases a fresh worktree on a remote-only branch supplied as fix/foo", async () => { + const { sourceRepo, remotePath, repoRoot } = await createClonedRepoWithRemote(); + const remoteSha = await pushRemoteOnlyBranch(sourceRepo, remotePath, "fix/foo", "remote-only.txt"); + + // The clone never learned the branch: no local ref and no remote-tracking ref. + await expect(readGit(repoRoot, ["rev-parse", "--verify", "fix/foo"])).rejects.toThrow(); + await expect(readGit(repoRoot, ["rev-parse", "--verify", "origin/fix/foo"])).rejects.toThrow(); + + const workspace = await realizeWorktreeForTest(repoRoot, "fix/foo"); + + expect(workspace.created).toBe(true); + expect(workspace.repoRef).toBe("origin/fix/foo"); + expect(workspace.baseRefSha).toBe(remoteSha); + expect(await readGit(workspace.cwd, ["rev-parse", "HEAD"])).toBe(remoteSha); + }); + + it("bases a fresh worktree on a remote-only branch supplied as origin/fix/foo", async () => { + const { sourceRepo, remotePath, repoRoot } = await createClonedRepoWithRemote(); + const remoteSha = await pushRemoteOnlyBranch(sourceRepo, remotePath, "fix/bar", "remote-only.txt"); + + await expect(readGit(repoRoot, ["rev-parse", "--verify", "origin/fix/bar"])).rejects.toThrow(); + + const workspace = await realizeWorktreeForTest(repoRoot, "origin/fix/bar"); + + expect(workspace.created).toBe(true); + expect(workspace.repoRef).toBe("origin/fix/bar"); + expect(workspace.baseRefSha).toBe(remoteSha); + expect(await readGit(workspace.cwd, ["rev-parse", "HEAD"])).toBe(remoteSha); + }); + + it("stops before git worktree add when the base ref is absent on origin", async () => { + const { repoRoot } = await createClonedRepoWithRemote(); + + const error = await realizeWorktreeForTest(repoRoot, "fix/does-not-exist").then( + () => null, + (caught: unknown) => caught, + ); + + expect(error).toBeInstanceOf(UnresolvedWorkspaceBaseRefError); + const unresolved = error as UnresolvedWorkspaceBaseRefError; + expect(unresolved.requestedRef).toBe("fix/does-not-exist"); + expect(unresolved.recoveryIdentityRef).toBe("origin/fix/does-not-exist"); + expect(unresolved.attemptedRefs).toEqual(["origin/fix/does-not-exist"]); + // No worktree directory was created for the fresh-create path. + await expect( + fs.stat(path.join(repoRoot, ".paperclip", "worktrees", "PAP-447-add-worktree-support")), + ).rejects.toThrow(); + }); + + it("gives equivalent spellings of one absent remote ref the same recovery identity", async () => { + const { repoRoot } = await createClonedRepoWithRemote(); + + // The unqualified form and the remote-tracking form name the same remote + // branch. Both must map to one `recoveryIdentityRef`, so recovery does not + // treat a spelling change as a new blocker. + const unqualified = await realizeWorktreeForTest(repoRoot, "fix/absent").then( + () => null, + (caught: unknown) => caught, + ); + const remoteTracking = await realizeWorktreeForTest(repoRoot, "origin/fix/absent").then( + () => null, + (caught: unknown) => caught, + ); + // The full remote-tracking spelling names the same branch as well. It must + // map to the same canonical recovery identity, not to its raw spelling. + const fullRemoteTracking = await realizeWorktreeForTest(repoRoot, "refs/remotes/origin/fix/absent").then( + () => null, + (caught: unknown) => caught, + ); + + expect(unqualified).toBeInstanceOf(UnresolvedWorkspaceBaseRefError); + expect(remoteTracking).toBeInstanceOf(UnresolvedWorkspaceBaseRefError); + expect(fullRemoteTracking).toBeInstanceOf(UnresolvedWorkspaceBaseRefError); + const unqualifiedError = unqualified as UnresolvedWorkspaceBaseRefError; + const remoteTrackingError = remoteTracking as UnresolvedWorkspaceBaseRefError; + const fullRemoteTrackingError = fullRemoteTracking as UnresolvedWorkspaceBaseRefError; + // Each error keeps its own operator spelling for the human notice. + expect(unqualifiedError.requestedRef).toBe("fix/absent"); + expect(remoteTrackingError.requestedRef).toBe("origin/fix/absent"); + expect(fullRemoteTrackingError.requestedRef).toBe("refs/remotes/origin/fix/absent"); + // All three share one canonical recovery identity. + expect(unqualifiedError.recoveryIdentityRef).toBe("origin/fix/absent"); + expect(remoteTrackingError.recoveryIdentityRef).toBe("origin/fix/absent"); + expect(fullRemoteTrackingError.recoveryIdentityRef).toBe("origin/fix/absent"); + }); + + it("surfaces an authenticated fetch failure as an unresolved base ref, not a crash", async () => { + const { repoRoot } = await createClonedRepoWithRemote(); + // Point origin at a path that no repository backs. The authenticated fetch + // fails, so the ref never resolves and the resolver reports the fetch error. + await runGit(repoRoot, ["remote", "set-url", "origin", path.join(os.tmpdir(), "paperclip-missing-remote.git")]); + + const error = await realizeWorktreeForTest(repoRoot, "fix/unreachable").then( + () => null, + (caught: unknown) => caught, + ); + + expect(error).toBeInstanceOf(UnresolvedWorkspaceBaseRefError); + const unresolved = error as UnresolvedWorkspaceBaseRefError; + expect(unresolved.requestedRef).toBe("fix/unreachable"); + expect(unresolved.fetchError).toEqual( + expect.stringContaining("Could not refresh base ref origin/fix/unreachable"), + ); + }); + it("rejects reusing an empty directory that only looks like a worktree because it sits inside the repo", async () => { const repoRoot = await createTempRepo(); const branchName = "PAP-447-add-worktree-support"; @@ -7477,14 +7603,26 @@ describeEmbeddedPostgres("workspace runtime startup reconciliation", () => { process.env.PAPERCLIP_HOME = paperclipHome; process.env.PAPERCLIP_INSTANCE_ID = `runtime-pnpm-reconcile-${randomUUID()}`; - const portProbe = net.createServer(); - await new Promise((resolve) => portProbe.listen(0, "127.0.0.1", resolve)); - const address = portProbe.address(); - const port = typeof address === "object" && address ? address.port : null; - await new Promise((resolve, reject) => { - portProbe.close((error) => error ? reject(error) : resolve()); - }); - if (!port) throw new Error("Failed to reserve pnpm reconciliation test port"); + // Reserve a port outside the runtime exposure app-port range (42000-42999). + // The reconciler stores this port on the row. A port inside that range makes + // the reconciler treat the row as an exposure reservation and report drift, + // so the live service never reaches the adoption path this test verifies. + const reservePort = async () => { + for (let attempt = 0; attempt < 100; attempt += 1) { + const probe = net.createServer(); + await new Promise((resolve) => probe.listen(0, "127.0.0.1", resolve)); + const address = probe.address(); + const candidate = typeof address === "object" && address ? address.port : null; + await new Promise((resolve, reject) => { + probe.close((error) => error ? reject(error) : resolve()); + }); + if (candidate && candidate <= 55_535 && (candidate < 42_000 || candidate > 42_999)) { + return candidate; + } + } + throw new Error("Failed to reserve pnpm reconciliation test port outside the broker range"); + }; + const port = await reservePort(); const companyId = randomUUID(); const projectId = randomUUID(); diff --git a/server/src/services/heartbeat.ts b/server/src/services/heartbeat.ts index 64cac948d0..e2ef031a56 100644 --- a/server/src/services/heartbeat.ts +++ b/server/src/services/heartbeat.ts @@ -142,9 +142,11 @@ import { persistAdapterManagedRuntimeServices, realizeExecutionWorkspace, releaseRuntimeServicesForRun, + isUnresolvedWorkspaceBaseRefError, type ExecutionWorkspaceInput, type RealizedExecutionWorkspace, type RuntimeServiceRef, + type UnresolvedWorkspaceBaseRefError, sanitizeRuntimeServiceBaseEnv, } from "./workspace-runtime.js"; import { @@ -459,6 +461,14 @@ const WORKSPACE_VALIDATION_FAILURE_CODE = "workspace_validation_failed"; const WORKSPACE_VALIDATION_RECOVERY_CAUSE = "workspace_validation_failed"; const CONFIGURATION_INCOMPLETE_FAILURE_CODE = "configuration_incomplete"; const CONFIGURATION_INCOMPLETE_RECOVERY_CAUSE = "configuration_incomplete"; +// Error codes that mark a pre-dispatch setup failure. The adapter process never +// started, so no agent could post an issue comment. The setup catch writes one +// of these codes when a failure happens before `adapter.execute`. +const PRE_ADAPTER_SETUP_FAILURE_CODES = new Set([ + "setup_failed", + CONFIGURATION_INCOMPLETE_FAILURE_CODE, + WORKSPACE_VALIDATION_FAILURE_CODE, +]); const EXECUTION_REVIEW_PARTICIPANT_RECOVERY_RETRY_REASON = "execution_review_participant_recovery"; const EXECUTION_REVIEW_PARTICIPANT_RECOVERY_WAKE_REASON = "execution_review_participant_recovery"; const EXECUTION_REVIEW_PARTICIPANT_RECOVERY_CAUSE = "execution_review_participant_recovery"; @@ -542,6 +552,35 @@ export class ConfigurationIncompleteFailure extends Error { } } +// Build the configuration-incomplete result payload for a workspace base ref +// that never resolved to a commit. The setup catch maps this to errorCode +// `configuration_incomplete`, so the recovery path routes it to a human owner +// instead of a dispatched-then-failed run. The `fingerprint` uses the canonical +// remote ref, not the operator spelling. Two equivalent spellings of one remote +// branch (`fix/foo` and `origin/fix/foo`) share one fingerprint, so a repeated +// failure reuses one active recovery action and does not reset the attempt +// count or post a duplicate notice. A different branch makes a new action. +function buildUnresolvedWorkspaceBaseRefResultJson( + run: typeof heartbeatRuns.$inferSelect, + error: UnresolvedWorkspaceBaseRefError, +): Record { + const context = parseObject(run.contextSnapshot); + return { + configurationIncomplete: { + reason: "workspace_base_ref_unresolved", + companyId: run.companyId, + agentId: run.agentId, + issueId: readNonEmptyString(context.issueId) ?? null, + projectId: readNonEmptyString(context.projectId) ?? null, + requestedRef: error.requestedRef, + attemptedRefs: error.attemptedRefs, + fetchError: error.fetchError, + fingerprint: `workspace_base_ref:${error.recoveryIdentityRef}`, + missingBindings: [], + }, + }; +} + export interface SharedWorkspaceHolder { runId: string; agentId: string; @@ -10074,6 +10113,22 @@ export function heartbeatService(db: Db, options: HeartbeatServiceOptions = {}) return { outcome: "not_applicable" as const, queuedRun: null }; } + // A pre-dispatch setup failure means the adapter process never started (for + // example an unresolved workspace base ref). No agent could run, so no agent + // could post an issue comment. A missing-comment retry cannot help and would + // loop the identical pre-adapter failure, so mark the policy not_applicable + // and queue nothing. + if (run.errorCode != null && PRE_ADAPTER_SETUP_FAILURE_CODES.has(run.errorCode)) { + if (run.issueCommentStatus !== "not_applicable") { + await patchRunIssueCommentStatus(run.id, { + issueCommentStatus: "not_applicable", + issueCommentSatisfiedByCommentId: null, + issueCommentRetryQueuedAt: null, + }); + } + return { outcome: "not_applicable" as const, queuedRun: null }; + } + const postedComment = await findRunIssueComment(run.id, run.companyId, issueId); if (postedComment) { await patchRunIssueCommentStatus(run.id, { @@ -16677,11 +16732,17 @@ export function heartbeatService(db: Db, options: HeartbeatServiceOptions = {}) // recovery path routes it to a human owner instead of looping retries. const workspaceValidationSetupFailure = isWorkspaceValidationFailure(outerErr) ? outerErr : null; const configurationIncompleteSetupFailure = isConfigurationIncompleteFailure(outerErr) ? outerErr : null; + // A remote-only base ref that never resolved is a known pre-dispatch + // configuration gap, not an opaque setup crash. Map it to the same + // configuration-incomplete code so the recovery path routes it to a + // human owner and bounds the repeat by its per-ref fingerprint. + const unresolvedBaseRefSetupFailure = isUnresolvedWorkspaceBaseRefError(outerErr) ? outerErr : null; const recordedResponsibleUserDenialCode = normalizeResponsibleUserDenialCode((await getRun(runId).catch(() => null))?.errorCode); const setupFailureErrorCode = workspaceValidationSetupFailure?.code ?? configurationIncompleteSetupFailure?.code ?? + (unresolvedBaseRefSetupFailure ? CONFIGURATION_INCOMPLETE_FAILURE_CODE : null) ?? recordedResponsibleUserDenialCode ?? "setup_failed"; logger.error({ err: outerErr, runId }, "heartbeat execution setup failed"); @@ -16695,7 +16756,11 @@ export function heartbeatService(db: Db, options: HeartbeatServiceOptions = {}) errorCode: setupFailureErrorCode, errorMessage: message, resultJson: - workspaceValidationSetupFailure?.resultJson ?? configurationIncompleteSetupFailure?.resultJson ?? null, + workspaceValidationSetupFailure?.resultJson ?? + configurationIncompleteSetupFailure?.resultJson ?? + (unresolvedBaseRefSetupFailure + ? buildUnresolvedWorkspaceBaseRefResultJson(run, unresolvedBaseRefSetupFailure) + : null), }), } : {}), }).catch(() => ({ run: null, updated: false as const })); diff --git a/server/src/services/issue-recovery-actions.ts b/server/src/services/issue-recovery-actions.ts index f9755274bd..dfb4f3978e 100644 --- a/server/src/services/issue-recovery-actions.ts +++ b/server/src/services/issue-recovery-actions.ts @@ -36,6 +36,11 @@ export type UpsertIssueRecoveryActionInput = { timeoutAt?: Date | null; lastAttemptAt?: Date | null; attemptCount?: number; + // When true, a change of (cause, fingerprint) does not overwrite the active + // action in place. The service resolves the prior action and inserts a new + // one. The new failure then gets a distinct recovery identity and a fresh + // operator notice, and the prior identity stays as a resolved record. + supersedeOnIdentityChange?: boolean; }; export type ResolveIssueRecoveryActionInput = { @@ -179,6 +184,81 @@ export function issueRecoveryActionService(db: Db) { return upsertSourceScopedUnlocked(input, retryCount + 1); } + function buildInsertValues( + input: UpsertIssueRecoveryActionInput, + ownerType: IssueRecoveryActionOwnerType, + now: Date, + ) { + return { + companyId: input.companyId, + sourceIssueId: input.sourceIssueId, + recoveryIssueId: input.recoveryIssueId ?? null, + kind: input.kind, + status: "active" as const, + ownerType, + ownerAgentId: input.ownerAgentId ?? null, + ownerUserId: input.ownerUserId ?? null, + previousOwnerAgentId: input.previousOwnerAgentId ?? null, + returnOwnerAgentId: input.returnOwnerAgentId ?? null, + cause: input.cause, + fingerprint: input.fingerprint, + evidence: input.evidence ?? {}, + nextAction: input.nextAction, + wakePolicy: input.wakePolicy ?? null, + monitorPolicy: input.monitorPolicy ?? null, + attemptCount: input.attemptCount ?? 1, + maxAttempts: input.maxAttempts ?? null, + timeoutAt: input.timeoutAt ?? null, + lastAttemptAt: input.lastAttemptAt ?? now, + }; + } + + // Resolve the prior active action, then insert a new one in one transaction. + // The prior identity stays as a cancelled record and the new failure gets a + // fresh action row with its own id. The partial unique index on the active + // status stays satisfied because only the new row is active at commit. + async function supersedePriorAndInsert( + input: UpsertIssueRecoveryActionInput, + priorActionId: string, + ownerType: IssueRecoveryActionOwnerType, + now: Date, + retryCount: number, + ): Promise { + try { + const created = await db.transaction(async (tx) => { + const [superseded] = await tx + .update(issueRecoveryActions) + .set({ + status: "cancelled", + outcome: "cancelled", + resolutionNote: "A new failure with a different identity superseded this recovery action.", + resolvedAt: now, + updatedAt: now, + }) + .where( + and( + eq(issueRecoveryActions.id, priorActionId), + inArray(issueRecoveryActions.status, [...ACTIVE_RECOVERY_ACTION_STATUSES]), + ), + ) + .returning(); + // Another writer resolved the prior action first. Abort and retry the + // whole upsert so the retry reads the current active state. + if (!superseded) return null; + const [row] = await tx + .insert(issueRecoveryActions) + .values(buildInsertValues(input, ownerType, now)) + .returning(); + return row ?? null; + }); + if (!created) return retryUpsertSourceScoped(input, retryCount); + return toReadModel(created); + } catch (error) { + if (!isUniqueRecoveryActionConflict(error)) throw error; + return retryUpsertSourceScoped(input, retryCount, error); + } + } + async function upsertSourceScopedUnlocked( input: UpsertIssueRecoveryActionInput, retryCount = 0, @@ -187,6 +267,15 @@ export function issueRecoveryActionService(db: Db) { const now = new Date(); const ownerType = input.ownerType ?? (input.ownerAgentId ? "agent" : "board"); if (existing) { + // A distinct failure identity must not overwrite the active action of a + // prior identity. Resolve the prior action and insert a new one, so the + // operator gets a new notice for the new failure. + if ( + input.supersedeOnIdentityChange && + (existing.cause !== input.cause || existing.fingerprint !== input.fingerprint) + ) { + return supersedePriorAndInsert(input, existing.id, ownerType, now, retryCount); + } const [updated] = await db .update(issueRecoveryActions) .set({ @@ -229,28 +318,7 @@ export function issueRecoveryActionService(db: Db) { try { const [created] = await db .insert(issueRecoveryActions) - .values({ - companyId: input.companyId, - sourceIssueId: input.sourceIssueId, - recoveryIssueId: input.recoveryIssueId ?? null, - kind: input.kind, - status: "active", - ownerType, - ownerAgentId: input.ownerAgentId ?? null, - ownerUserId: input.ownerUserId ?? null, - previousOwnerAgentId: input.previousOwnerAgentId ?? null, - returnOwnerAgentId: input.returnOwnerAgentId ?? null, - cause: input.cause, - fingerprint: input.fingerprint, - evidence: input.evidence ?? {}, - nextAction: input.nextAction, - wakePolicy: input.wakePolicy ?? null, - monitorPolicy: input.monitorPolicy ?? null, - attemptCount: input.attemptCount ?? 1, - maxAttempts: input.maxAttempts ?? null, - timeoutAt: input.timeoutAt ?? null, - lastAttemptAt: input.lastAttemptAt ?? now, - }) + .values(buildInsertValues(input, ownerType, now)) .returning(); return toReadModel(created!); } catch (error) { diff --git a/server/src/services/recovery/service.ts b/server/src/services/recovery/service.ts index b299ac7352..8b00c7d07b 100644 --- a/server/src/services/recovery/service.ts +++ b/server/src/services/recovery/service.ts @@ -300,6 +300,11 @@ function readWorkspaceValidationFingerprint(latestRun: LatestIssueRun): string | return readNonEmptyString(payload?.fingerprint); } +function readConfigurationIncompleteFingerprint(latestRun: LatestIssueRun): string | null { + const payload = parseObject(parseObject(latestRun?.resultJson).configurationIncomplete); + return readNonEmptyString(payload?.fingerprint); +} + type WatchdogDecisionActor = | { type: "board"; userId?: string | null; runId?: string | null } | { type: "agent"; agentId?: string | null; runId?: string | null } @@ -2867,6 +2872,23 @@ export function recoveryService(db: Db, deps: { enqueueWakeup: RecoveryWakeup }) ].join(":"); } } + // A configuration-incomplete failure that carries a stable identity (for + // example an unresolved workspace base ref) dedupes per that identity, so a + // different requested ref makes a new recovery action while the same ref + // reuses one. Configuration gaps with no fingerprint fall back to the + // issue-and-cause scope below. + if (input.recoveryCause === "configuration_incomplete") { + const configurationFingerprint = readConfigurationIncompleteFingerprint(input.latestRun); + if (configurationFingerprint) { + return [ + "source_scoped_recovery", + input.issue.companyId, + input.issue.id, + input.recoveryCause, + configurationFingerprint, + ].join(":"); + } + } return [ "source_scoped_recovery", input.issue.companyId, @@ -2925,6 +2947,11 @@ export function recoveryService(db: Db, deps: { enqueueWakeup: RecoveryWakeup }) const action = await recoveryActionsSvc.upsertSourceScoped({ companyId: input.issue.companyId, sourceIssueId: input.issue.id, + // A configuration-incomplete failure carries a per-identity fingerprint + // (for example the unresolved workspace base ref). A different ref is a + // distinct blocker, so it must get a new recovery action and notify the + // operator, not overwrite the active action of the prior ref. + supersedeOnIdentityChange: recoveryCause === "configuration_incomplete", kind: strandedRecoveryActionKind(recoveryCause), ownerType: recoveryCause === "provider_quota" && !ownerAgentId ? "system" : ownerAgentId ? "agent" : "board", ownerAgentId, diff --git a/server/src/services/workspace-runtime.ts b/server/src/services/workspace-runtime.ts index 261e00613c..02d4ca4b74 100644 --- a/server/src/services/workspace-runtime.ts +++ b/server/src/services/workspace-runtime.ts @@ -2364,26 +2364,116 @@ export async function ensureGitWorktreeBranchCoherent(input: { }; } +// A configured base ref that does not resolve to a commit, even after an +// authenticated fetch of its `origin/` counterpart. The caller must +// stop before `git worktree add` and raise a pre-dispatch configuration +// failure. `requestedRef` keeps the operator spelling for the human notice. +// `recoveryIdentityRef` is the canonical remote ref the resolver probed, so two +// equivalent spellings of one remote branch map to one recovery identity. +// `attemptedRefs` names each ref the resolver tried, and `fetchError` carries +// the first fetch warning (masked) when the fetch itself failed. +export class UnresolvedWorkspaceBaseRefError extends Error { + requestedRef: string; + recoveryIdentityRef: string; + attemptedRefs: string[]; + fetchError: string | null; + + constructor(input: { + requestedRef: string; + recoveryIdentityRef: string; + attemptedRefs: string[]; + fetchError?: string | null; + }) { + super( + `Configured workspace base ref "${input.requestedRef}" did not resolve to a commit on origin after an authenticated fetch.`, + ); + this.name = "UnresolvedWorkspaceBaseRefError"; + this.requestedRef = input.requestedRef; + this.recoveryIdentityRef = input.recoveryIdentityRef; + this.attemptedRefs = input.attemptedRefs; + this.fetchError = input.fetchError ?? null; + } +} + +export function isUnresolvedWorkspaceBaseRefError(error: unknown): error is UnresolvedWorkspaceBaseRefError { + return error instanceof UnresolvedWorkspaceBaseRefError; +} + +// A resolved base ref that the caller can pass to `git worktree add`, or an +// unresolved outcome that must stop the caller before it creates the worktree. +type AuthoritativeBaseRefResolution = + | { resolved: true; baseRef: string; warnings: string[]; refreshed: boolean } + | { + resolved: false; + requestedRef: string; + // The canonical remote ref the resolver probed for this branch, for + // example `origin/fix/foo`. Two equivalent spellings of one remote branch + // (`fix/foo` and `origin/fix/foo`) share this value, so recovery treats + // them as one identity. Two different branches get different values. + recoveryIdentityRef: string; + attemptedRefs: string[]; + warnings: string[]; + fetchError: string | null; + }; + // Resolve the authoritative base ref for a fresh worktree. A configured local // branch is mapped to its `origin/` counterpart so unpushed local -// divergence never leaks into the task branch; remote-tracking refs, SHAs, and -// tags are used verbatim, and an unset/`HEAD` base falls back to the detected -// default branch (which already prefers `origin/master`). +// divergence never leaks into the task branch; SHAs and tags are used verbatim, +// and an unset/`HEAD` base falls back to the detected default branch (which +// already prefers `origin/master`). +// +// A remote-only feature branch never has a local ref or a remote-tracking ref +// yet. The resolver fetches `origin/` with the authenticated helper, +// then re-checks the commit. This covers both the unqualified form (`fix/foo`) +// and the remote-tracking form (`origin/fix/foo`). A ref that still does not +// resolve returns `resolved: false`, so the caller stops before the worktree +// add instead of passing an invalid reference to git. async function resolveAuthoritativeBaseRef( repoRoot: string, configuredBaseRef: string | null, resolveGitAuth?: GitRemoteAuthProvider | null, -): Promise<{ baseRef: string; warnings: string[]; refreshed: boolean }> { +): Promise { const warnings: string[] = []; const detectOrHead = async () => (await detectDefaultBranch(repoRoot, resolveGitAuth)) ?? "HEAD"; const configured = configuredBaseRef?.trim(); if (!configured || configured === "HEAD") { - return { baseRef: await detectOrHead(), warnings, refreshed: false }; + return { resolved: true, baseRef: await detectOrHead(), warnings, refreshed: false }; } - if (parseRemoteTrackingRef(configured)) { - return { baseRef: configured, warnings, refreshed: false }; + // A remote-tracking ref supplied directly (for example `origin/fix/foo`). + // Use it verbatim when it already resolves. When it does not, fetch it once + // and re-check, then stop if it is still absent on the remote. + // + // `parseRemoteTrackingRef` only checks the `remote/branch` shape. An + // unqualified branch name that contains a slash (for example `fix/foo`) has + // the same shape but names no real remote, so it is not a remote-tracking + // ref. Gate this branch on the first segment naming an existing remote, and + // let a name like `fix/foo` fall through to the remote-only branch handling + // below. + const remoteTracking = parseRemoteTrackingRef(configured); + if (remoteTracking && await resolveBaseRefSha(repoRoot, configured)) { + return { resolved: true, baseRef: configured, warnings, refreshed: false }; + } + if (remoteTracking && await remoteExists(repoRoot, remoteTracking.remote)) { + const fetchWarnings = await refreshRemoteTrackingBaseRef(repoRoot, configured, resolveGitAuth); + warnings.push(...fetchWarnings); + if (await resolveBaseRefSha(repoRoot, configured)) { + return { resolved: true, baseRef: configured, warnings, refreshed: true }; + } + // Build the recovery identity from the parsed remote and branch. The raw + // ref and its remote-tracking spelling (`origin/fix/foo` and + // `refs/remotes/origin/fix/foo`) then share one recovery fingerprint, so + // recovery treats them as one identity instead of two. + const canonicalRemoteRef = `${remoteTracking.remote}/${remoteTracking.branch}`; + return { + resolved: false, + requestedRef: configured, + recoveryIdentityRef: canonicalRemoteRef, + attemptedRefs: [configured], + warnings, + fetchError: fetchWarnings[0] ?? null, + }; } if (await localBranchExists(repoRoot, configured)) { @@ -2392,17 +2482,37 @@ async function resolveAuthoritativeBaseRef( // the returned ref (see `refreshed`) so we never fetch the same ref twice. warnings.push(...await refreshRemoteTrackingBaseRef(repoRoot, remoteCandidate, resolveGitAuth)); if (await resolveBaseRefSha(repoRoot, remoteCandidate)) { - return { baseRef: remoteCandidate, warnings, refreshed: true }; + return { resolved: true, baseRef: remoteCandidate, warnings, refreshed: true }; } if (await remoteExists(repoRoot, "origin")) { warnings.push( `Configured base ref "${configured}" is a local branch with no matching origin/${configured}; basing the execution workspace on the local ref, which may include unpushed commits.`, ); } - return { baseRef: configured, warnings, refreshed: false }; + return { resolved: true, baseRef: configured, warnings, refreshed: false }; } - return { baseRef: configured, warnings, refreshed: false }; + // Fall-through: an unqualified ref (for example `fix/foo`) that is not `HEAD`, + // not a remote-tracking ref, and not a local branch. A full SHA or a tag that + // already resolves stays verbatim. Otherwise treat it as a remote-only branch + // name: fetch `origin/` and base the worktree on the remote counterpart. + if (await resolveBaseRefSha(repoRoot, configured)) { + return { resolved: true, baseRef: configured, warnings, refreshed: false }; + } + const remoteCandidate = `origin/${configured}`; + const fetchWarnings = await refreshRemoteTrackingBaseRef(repoRoot, remoteCandidate, resolveGitAuth); + warnings.push(...fetchWarnings); + if (await resolveBaseRefSha(repoRoot, remoteCandidate)) { + return { resolved: true, baseRef: remoteCandidate, warnings, refreshed: true }; + } + return { + resolved: false, + requestedRef: configured, + recoveryIdentityRef: remoteCandidate, + attemptedRefs: [remoteCandidate], + warnings, + fetchError: fetchWarnings[0] ?? null, + }; } // Auto-refresh a reused worktree to the latest base only when it is provably @@ -3117,11 +3227,15 @@ export async function realizeExecutionWorkspace(input: { const configuredBaseRef = typeof rawStrategy.baseRef === "string" && rawStrategy.baseRef.length > 0 ? rawStrategy.baseRef : input.base.repoRef ?? null; - const { - baseRef, - warnings: baseRefResolutionWarnings, - refreshed: baseRefAlreadyRefreshed, - } = await resolveAuthoritativeBaseRef(repoRoot, configuredBaseRef, input.resolveGitAuth); + const baseRefResolution = await resolveAuthoritativeBaseRef(repoRoot, configuredBaseRef, input.resolveGitAuth); + // Keep a usable base ref for the reuse and drift paths even when the ref is + // unresolved: those paths tolerate a null base-ref SHA and never run + // `git worktree add -b `. Only the fresh-create path below + // stops on an unresolved ref. `baseRefAlreadyRefreshed` is true for the + // unresolved case because the resolver already attempted the fetch. + const baseRef = baseRefResolution.resolved ? baseRefResolution.baseRef : baseRefResolution.requestedRef; + const baseRefResolutionWarnings = baseRefResolution.warnings; + const baseRefAlreadyRefreshed = baseRefResolution.resolved ? baseRefResolution.refreshed : true; const baseRefreshWarnings = [ ...baseRefResolutionWarnings, ...(baseRefAlreadyRefreshed ? [] : await refreshRemoteTrackingBaseRef(repoRoot, baseRef, input.resolveGitAuth)), @@ -3257,6 +3371,19 @@ export async function realizeExecutionWorkspace(input: { throw new Error(`Registered worktree for branch "${branchName}" at "${registeredBranchWorktree}" is not reusable${reason}.`); } + // No reusable worktree exists, so a fresh `git worktree add -b ` + // must run next. An unresolved base ref would make git fail with + // `fatal: invalid reference`. Stop here instead and raise a pre-dispatch + // configuration failure that the setup catch routes to a human owner. + if (!baseRefResolution.resolved) { + throw new UnresolvedWorkspaceBaseRefError({ + requestedRef: baseRefResolution.requestedRef, + recoveryIdentityRef: baseRefResolution.recoveryIdentityRef, + attemptedRefs: baseRefResolution.attemptedRefs, + fetchError: baseRefResolution.fetchError, + }); + } + try { await recordGitOperation(input.recorder, { phase: "worktree_prepare", diff --git a/server/vitest.config.ts b/server/vitest.config.ts index 0356d4787b..faa0c7b0e1 100644 --- a/server/vitest.config.ts +++ b/server/vitest.config.ts @@ -13,6 +13,15 @@ export default defineConfig({ // mirrors it for the same reason. hookTimeout: 30000, teardownTimeout: 30000, + // The route/authz suites import very large modules (for example + // src/routes/issues.ts and its dependency graph). The first test in each + // file pays the one-time transform cost inside its own timeout budget. On + // the loaded serial shard (maxWorkers=1) that cost can cross vitest's + // default 5s testTimeout and fail the first test, which also lets its + // fire-and-forget wake leak into the next test. Give each test generous + // headroom; 15s is far above the observed module-load cost yet still + // catches a genuinely hung test well inside the 20 minute job limit. + testTimeout: 15000, isolate: true, maxConcurrency: 1, maxWorkers: 1,