fix(heartbeat): reuse sessions across execution handoffs (#9917)
## 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>
This commit is contained in:
parent
b90da4d115
commit
8774909361
|
|
@ -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" },
|
||||
|
|
|
|||
|
|
@ -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", () => {
|
||||
|
|
|
|||
|
|
@ -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)) {
|
||||
|
|
|
|||
Loading…
Reference in New Issue