From 877490936199c1149117a4b28c90fc5048bd7061 Mon Sep 17 00:00:00 2001 From: santastabber Date: Tue, 18 Aug 2026 07:30:47 +1000 Subject: [PATCH] fix(heartbeat): reuse sessions across execution handoffs (#9917) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Thinking Path > - Paperclip uses durable task sessions so local adapters can resume work across sequential heartbeat runs. > - `execution_review_requested` and `execution_changes_requested` are issue-local execution-policy handoffs, not new task assignments. > - The existing `agent_task_sessions` lookup, adapter session codec, workspace resolution, and effective config freshness checks already decide whether reuse is safe. > - Treating those two handoff wake reasons as unconditional fresh-session boundaries discards a valid saved task session before adapter resume can be attempted. > - This makes Dev → CodeReview → Dev loops repeatedly cold-start even when task, issue, agent, adapter, workspace, and config identity are unchanged. > - The fix is to let normal review/change-request handoffs reach the durable task-session path while preserving explicit fresh-session and unsafe-boundary resets. ## Linked Issues or Issue Description Fixes #8246. cc @cryppadotta — this is the narrow handoff-session policy change discussed there: normal `execution_review_requested` / `execution_changes_requested` wakes no longer force a fresh task session by wake reason alone, while assignment, approval, review-participant recovery, timer wakes, explicit `forceFreshSession`, and config/workspace/model/session freshness still keep their safety boundaries. ## What Changed - Removed normal `execution_review_requested` and `execution_changes_requested` from the unconditional task-session reset policy. - Kept fresh-session boundaries for: - `issue_assigned` - `execution_approval_requested` - `execution_review_participant_recovery` - `heartbeat_timer` - explicit `forceFreshSession` - existing config/model/workspace/session freshness reset paths - Updated heartbeat session-policy tests so execution handoffs are resume-eligible by wake reason alone. - Preserved PF-4 timer-wake behavior and its explicit reset reason. ## Verification - `npx pnpm@9.15.4 exec vitest run server/src/__tests__/heartbeat-workspace-session.test.ts server/src/__tests__/heartbeat-timer-wake-session-reset-pf4.test.ts server/src/__tests__/codex-local-execute.test.ts server/src/__tests__/issue-comment-reopen-routes.test.ts --reporter=verbose` — 220 tests passed. - `npx pnpm@9.15.4 --filter @paperclipai/server typecheck` — passed. - `git diff --check` — passed. - `coderabbit review --agent -t committed --base origin/master` — 0 findings. ## Risks - Moderate behavior change in session-boundary policy: normal review/change-request handoffs may now reuse a saved per-task session when the existing identity/freshness checks pass. - Safety boundaries remain in place for new assignments, approval gates, review-participant recovery, timer/discovery wakes, explicit fresh-session requests, and config/model/workspace/session drift. - If a saved session is stale or incompatible, existing freshness/resume fallback behavior still handles reset/fresh execution. > For core feature work, check ROADMAP.md first and discuss it in #dev before opening the PR. Feature PRs that overlap with planned core work may need to be redirected — check the roadmap first. See CONTRIBUTING.md. ## Model Used OpenAI GPT-5.5. Tool use and local verification were enabled. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links - [x] My branch name describes the change and contains no internal Paperclip ticket ID - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes — N/A, server policy/test-only change - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge Co-authored-by: santastabber <184111696+santastabber@users.noreply.github.com> --- ...tbeat-timer-wake-session-reset-pf4.test.ts | 31 +++++++++++++------ .../heartbeat-workspace-session.test.ts | 8 ++--- server/src/services/heartbeat.ts | 4 --- 3 files changed, 25 insertions(+), 18 deletions(-) diff --git a/server/src/__tests__/heartbeat-timer-wake-session-reset-pf4.test.ts b/server/src/__tests__/heartbeat-timer-wake-session-reset-pf4.test.ts index b8fd03bd63..ca419d9e0c 100644 --- a/server/src/__tests__/heartbeat-timer-wake-session-reset-pf4.test.ts +++ b/server/src/__tests__/heartbeat-timer-wake-session-reset-pf4.test.ts @@ -30,17 +30,25 @@ describe("PF-4 shouldResetTaskSessionForWake", () => { ).toBe(false); }); - it("still resets for the existing reset reasons", () => { + it("keeps assignment, approval, and review-participant recovery as fresh boundaries", () => { for (const wakeReason of [ "issue_assigned", - "execution_review_requested", "execution_approval_requested", - "execution_changes_requested", + "execution_review_participant_recovery", ] as const) { expect(shouldResetTaskSessionForWake({ wakeReason })).toBe(true); } }); + it("does not reset execution handoffs by wake reason alone", () => { + for (const wakeReason of [ + "execution_review_requested", + "execution_changes_requested", + ] as const) { + expect(shouldResetTaskSessionForWake({ wakeReason })).toBe(false); + } + }); + it("still respects forceFreshSession === true", () => { expect(shouldResetTaskSessionForWake({ forceFreshSession: true })).toBe(true); }); @@ -80,19 +88,21 @@ describe("PF-4 describeSessionResetReason", () => { ).toBeNull(); }); - it("returns the existing reasons for the existing reset triggers", () => { + it("returns reasons for wake reasons that still force a fresh task session", () => { expect(describeSessionResetReason({ wakeReason: "issue_assigned" })).toBe( "wake reason is issue_assigned", ); - expect(describeSessionResetReason({ wakeReason: "execution_review_requested" })).toBe( - "wake reason is execution_review_requested", - ); expect(describeSessionResetReason({ wakeReason: "execution_approval_requested" })).toBe( "wake reason is execution_approval_requested", ); - expect(describeSessionResetReason({ wakeReason: "execution_changes_requested" })).toBe( - "wake reason is execution_changes_requested", - ); + expect( + describeSessionResetReason({ wakeReason: "execution_review_participant_recovery" }), + ).toBe("wake reason is execution_review_participant_recovery"); + }); + + it("does not report review/change-request handoffs as reset reasons", () => { + expect(describeSessionResetReason({ wakeReason: "execution_review_requested" })).toBeNull(); + expect(describeSessionResetReason({ wakeReason: "execution_changes_requested" })).toBeNull(); }); it("returns the forceFreshSession message when explicitly requested", () => { @@ -116,6 +126,7 @@ describe("PF-4 describeSessionResetReason", () => { { wakeReason: "issue_assigned" }, { wakeReason: "execution_review_requested" }, { wakeReason: "execution_approval_requested" }, + { wakeReason: "execution_review_participant_recovery" }, { wakeReason: "execution_changes_requested" }, { forceFreshSession: true }, { wakeReason: "issue_commented" }, diff --git a/server/src/__tests__/heartbeat-workspace-session.test.ts b/server/src/__tests__/heartbeat-workspace-session.test.ts index 0495f3bd93..963c10ad2f 100644 --- a/server/src/__tests__/heartbeat-workspace-session.test.ts +++ b/server/src/__tests__/heartbeat-workspace-session.test.ts @@ -1791,16 +1791,16 @@ describe("shouldResetTaskSessionForWake", () => { expect(shouldResetTaskSessionForWake({ wakeReason: "issue_assigned" })).toBe(true); }); - it("resets session context on execution review wakes", () => { - expect(shouldResetTaskSessionForWake({ wakeReason: "execution_review_requested" })).toBe(true); + it("preserves session context on execution review handoff wakes", () => { + expect(shouldResetTaskSessionForWake({ wakeReason: "execution_review_requested" })).toBe(false); }); it("resets session context on execution approval wakes", () => { expect(shouldResetTaskSessionForWake({ wakeReason: "execution_approval_requested" })).toBe(true); }); - it("resets session context on execution changes-requested wakes", () => { - expect(shouldResetTaskSessionForWake({ wakeReason: "execution_changes_requested" })).toBe(true); + it("preserves session context on execution changes-requested handoff wakes", () => { + expect(shouldResetTaskSessionForWake({ wakeReason: "execution_changes_requested" })).toBe(false); }); it("preserves session context on timer heartbeats", () => { diff --git a/server/src/services/heartbeat.ts b/server/src/services/heartbeat.ts index b92f81358b..e3c3b34454 100644 --- a/server/src/services/heartbeat.ts +++ b/server/src/services/heartbeat.ts @@ -4122,10 +4122,8 @@ export function shouldResetTaskSessionForWake( const wakeReason = readNonEmptyString(contextSnapshot?.wakeReason); if ( wakeReason === "issue_assigned" || - wakeReason === "execution_review_requested" || wakeReason === EXECUTION_REVIEW_PARTICIPANT_RECOVERY_WAKE_REASON || wakeReason === "execution_approval_requested" || - wakeReason === "execution_changes_requested" || // PF-4: unscoped timer wakes are exploratory ("any new work?") and should // not accumulate low-value inbox scans. Issue-scoped timer wakes are // continuation work, so reuse their task session to avoid paying the full @@ -4235,12 +4233,10 @@ export function describeSessionResetReason( const wakeReason = readNonEmptyString(contextSnapshot?.wakeReason); if (wakeReason === "issue_assigned") return "wake reason is issue_assigned"; - if (wakeReason === "execution_review_requested") return "wake reason is execution_review_requested"; if (wakeReason === EXECUTION_REVIEW_PARTICIPANT_RECOVERY_WAKE_REASON) { return `wake reason is ${EXECUTION_REVIEW_PARTICIPANT_RECOVERY_WAKE_REASON}`; } if (wakeReason === "execution_approval_requested") return "wake reason is execution_approval_requested"; - if (wakeReason === "execution_changes_requested") return "wake reason is execution_changes_requested"; // PF-4: paired with shouldResetTaskSessionForWake — keep the reason wording // explicit so run logs make session reuse/reset behavior legible. if (wakeReason === "heartbeat_timer" && !deriveTaskKey(contextSnapshot, null)) {