From b91859c258f5e13096abb55ab9425630807cc2dd Mon Sep 17 00:00:00 2001 From: Chris Farhood Date: Thu, 23 Apr 2026 23:58:23 +0000 Subject: [PATCH] refactor: extract classifyOrphan helper with decision matrix (#8) Co-Authored-By: Claude Sonnet Co-Authored-By: Paperclip --- src/server/execute.test.ts | 70 +++++++++++++++++++------------------- src/server/execute.ts | 67 ++++++++++++++++++++---------------- 2 files changed, 73 insertions(+), 64 deletions(-) diff --git a/src/server/execute.test.ts b/src/server/execute.test.ts index 1b4b30a..638cb52 100644 --- a/src/server/execute.test.ts +++ b/src/server/execute.test.ts @@ -15,7 +15,7 @@ vi.mock("./k8s-client.js", () => ({ resetCache: vi.fn(), })); -const { isK8s404, buildPartialRunError, isReattachableOrphan, describePodTerminatedError, streamPodLogsOnce } = await import("./execute.js"); +const { isK8s404, buildPartialRunError, classifyOrphan, describePodTerminatedError, streamPodLogsOnce } = await import("./execute.js"); function makeJob(opts: { runId?: string; @@ -146,59 +146,59 @@ describe("buildPartialRunError", () => { }); }); -describe("isReattachableOrphan", () => { - const agentId = "agent-abc"; +describe("classifyOrphan", () => { const taskId = "task-xyz"; const sessionId = "sess-123"; - it("returns true when agent/task/session all match and Job is not terminal", () => { - const job = makeJob({ agentId, taskId, sessionId, runId: "old-run" }); - expect(isReattachableOrphan(job, { agentId, taskId, sessionId })).toBe(true); + // --- Happy path: reattach --- + it("returns reattach when taskId matches and both sessionIds match", () => { + const job = makeJob({ taskId, sessionId }); + expect(classifyOrphan(job, { taskId, sessionId })).toBe("reattach"); }); - it("returns false when the Job is already Complete", () => { - const job = makeJob({ agentId, taskId, sessionId, runId: "old-run", terminal: true }); - expect(isReattachableOrphan(job, { agentId, taskId, sessionId })).toBe(false); + it("returns reattach when taskId matches and expected sessionId is null (missing on current side)", () => { + const job = makeJob({ taskId, sessionId }); + expect(classifyOrphan(job, { taskId, sessionId: null })).toBe("reattach"); }); - it("returns false when expected taskId is null (caller couldn't derive one)", () => { - const job = makeJob({ agentId, taskId, sessionId }); - expect(isReattachableOrphan(job, { agentId, taskId: null, sessionId })).toBe(false); + it("returns reattach when taskId matches and job has no session-id label (missing on job side)", () => { + const job = makeJob({ taskId }); + expect(classifyOrphan(job, { taskId, sessionId })).toBe("reattach"); }); - it("returns false when expected sessionId is null", () => { - const job = makeJob({ agentId, taskId, sessionId }); - expect(isReattachableOrphan(job, { agentId, taskId, sessionId: null })).toBe(false); + it("returns reattach when taskId matches and neither side has a sessionId", () => { + const job = makeJob({ taskId }); + expect(classifyOrphan(job, { taskId, sessionId: null })).toBe("reattach"); }); - it("returns false when agent id doesn't match", () => { - const job = makeJob({ agentId: "agent-other", taskId, sessionId }); - expect(isReattachableOrphan(job, { agentId, taskId, sessionId })).toBe(false); + // --- Block: task unknown --- + it("returns block_task_unknown when expected taskId is null", () => { + const job = makeJob({ taskId, sessionId }); + expect(classifyOrphan(job, { taskId: null, sessionId })).toBe("block_task_unknown"); }); - it("returns false when task id doesn't match", () => { - const job = makeJob({ agentId, taskId: "task-other", sessionId }); - expect(isReattachableOrphan(job, { agentId, taskId, sessionId })).toBe(false); + it("returns block_task_unknown when job has no task-id label", () => { + const job = makeJob({ sessionId }); + expect(classifyOrphan(job, { taskId, sessionId })).toBe("block_task_unknown"); }); - it("returns false when session id doesn't match", () => { - const job = makeJob({ agentId, taskId, sessionId: "sess-other" }); - expect(isReattachableOrphan(job, { agentId, taskId, sessionId })).toBe(false); + // --- Block: task mismatch --- + it("returns block_task_mismatch when both sides have taskId but they differ", () => { + const job = makeJob({ taskId: "task-other", sessionId }); + expect(classifyOrphan(job, { taskId, sessionId })).toBe("block_task_mismatch"); }); - it("returns false when the Job is from a different adapter type", () => { - const job = makeJob({ agentId, taskId, sessionId, adapterType: "claude_local" }); - expect(isReattachableOrphan(job, { agentId, taskId, sessionId })).toBe(false); + // --- Block: session mismatch --- + it("returns block_session_mismatch when taskId matches but sessionIds differ", () => { + const job = makeJob({ taskId, sessionId: "sess-other" }); + expect(classifyOrphan(job, { taskId, sessionId })).toBe("block_session_mismatch"); }); - it("returns false when Job has no task-id label (labels were introduced in FAR-124)", () => { - const job = makeJob({ agentId, sessionId }); - expect(isReattachableOrphan(job, { agentId, taskId, sessionId })).toBe(false); - }); - - it("returns false when Job has no session-id label", () => { - const job = makeJob({ agentId, taskId }); - expect(isReattachableOrphan(job, { agentId, taskId, sessionId })).toBe(false); + // --- Terminal orphan (caller filters these before classifyOrphan) --- + it("returns reattach for terminal job (caller is responsible for filtering terminals)", () => { + const job = makeJob({ taskId, sessionId, terminal: true }); + // classifyOrphan does not check terminal status — that is the caller's job + expect(classifyOrphan(job, { taskId, sessionId })).toBe("reattach"); }); }); diff --git a/src/server/execute.ts b/src/server/execute.ts index 53a1e3b..728887c 100644 --- a/src/server/execute.ts +++ b/src/server/execute.ts @@ -89,30 +89,46 @@ export function buildPartialRunError( : `Claude exited with code ${exitCode ?? -1}`; } +export type OrphanClassification = + | "reattach" + | "block_session_mismatch" + | "block_task_mismatch" + | "block_task_unknown"; + /** - * Evaluate an orphaned K8s Job (one whose `paperclip.io/run-id` label does - * not match the current runId) as a potential reattach target. A Job is - * reattachable when it belongs to the same agent, same task, and same resume - * session as the current run — meaning the previous Paperclip instance was - * mid-stream on the exact piece of work this new run was dispatched to do. + * Classify a non-terminal orphaned K8s Job (one whose `paperclip.io/run-id` + * label does not match the current runId but does belong to this agent) as a + * reattach candidate or a block reason. + * + * Decision matrix: + * - taskId mismatch (both present, different values) → block_task_mismatch + * - taskId missing on either side → block_task_unknown + * - taskId match + both have sessionId + sessionIds differ → block_session_mismatch + * - taskId match + one or both sides missing sessionId → reattach (reconcile) + * - taskId match + both have sessionId + sessionIds match → reattach (happy path) + * * Exported for unit tests. */ -export function isReattachableOrphan( +export function classifyOrphan( job: k8s.V1Job, - expected: { agentId: string; taskId: string | null; sessionId: string | null }, -): boolean { - if (!expected.taskId || !expected.sessionId) return false; + expected: { taskId: string | null; sessionId: string | null }, +): OrphanClassification { const labels = job.metadata?.labels ?? {}; - if (labels["paperclip.io/adapter-type"] !== "claude_k8s") return false; - if (labels["paperclip.io/agent-id"] !== expected.agentId) return false; - if (labels["paperclip.io/task-id"] !== expected.taskId) return false; - if (labels["paperclip.io/session-id"] !== expected.sessionId) return false; - const conditions = job.status?.conditions ?? []; - const terminal = conditions.some( - (c) => (c.type === "Complete" || c.type === "Failed") && c.status === "True", - ); - if (terminal) return false; - return true; + const jobTaskId = labels["paperclip.io/task-id"] ?? null; + const jobSessionId = labels["paperclip.io/session-id"] ?? null; + + // taskId missing on either side + if (!expected.taskId || !jobTaskId) return "block_task_unknown"; + + // taskId mismatch + if (expected.taskId !== jobTaskId) return "block_task_mismatch"; + + // taskId matches — check sessionId + if (expected.sessionId && jobSessionId && expected.sessionId !== jobSessionId) { + return "block_session_mismatch"; + } + + return "reattach"; } /** @@ -553,18 +569,11 @@ export async function execute(ctx: AdapterExecutionContext): Promise (j.metadata?.labels?.["paperclip.io/run-id"] ?? "") === runId, ); - // Pick the most recent reattachable orphan — same agent + task + session, - // not terminal. Only one target is chosen; any other orphans get - // cleaned up as before. + // Pick the most recent reattachable orphan — same task + session, not + // terminal. Only one target is chosen; any other orphans get cleaned up. if (reattachOrphanedJobs && orphaned.length > 0) { const candidates = orphaned - .filter((j) => - isReattachableOrphan(j, { - agentId, - taskId: currentTaskLabel, - sessionId: currentSessionLabel, - }), - ) + .filter((j) => classifyOrphan(j, { taskId: currentTaskLabel, sessionId: currentSessionLabel }) === "reattach") .sort((a, b) => { const at = new Date(a.metadata?.creationTimestamp ?? 0).getTime(); const bt = new Date(b.metadata?.creationTimestamp ?? 0).getTime();