feat(execution): execution_approved wake reason + self-approval fallback#4429
feat(execution): execution_approved wake reason + self-approval fallback#4429ramonmatias19 wants to merge 1 commit intopaperclipai:masterfrom
Conversation
Greptile SummaryThis PR adds an The PR description does not follow the required template from Confidence Score: 4/5Code logic is correct and well-tested; blocked on PR template compliance required before merge per CONTRIBUTING.md All code-level findings are P2 (the approved-prompt coupling to
Important Files Changed
Prompt To Fix All With AIThis is a comment left during a code review.
Path: server/src/__tests__/issue-execution-policy.test.ts
Line: 1261
Comment:
**Missing test for `shouldResetTaskSessionForWake` with new wake reason**
The existing test suite in `heartbeat-workspace-session.test.ts` has explicit cases for `execution_review_requested`, `execution_approval_requested`, and `execution_changes_requested`, but no case for the newly added `execution_approved` reason. Adding a parallel test there would ensure consistency and guard against regressions if the function is refactored.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: packages/adapter-utils/src/server-utils.ts
Line: 691-704
Comment:
**Approved-prompt fires on `lastDecisionOutcome` alone, not tied to wake reason**
The condition `executionStage.lastDecisionOutcome === "approved"` is used to decide which executor prompt to show, but the `buildCompletedState` function sets `lastDecisionOutcome: "approved"` as a permanent field even if the state later transitions elsewhere. If an executor receives a non-`execution_approved` wake while the last decision happens to be `"approved"` (e.g. a liveness / comment wake during a completed-but-not-yet-acknowledged window), they will see the completion prompt instead of a general-purpose one.
Tying this branch to a wake-reason check (`executionStage.wakeRole === "executor" && wakeReason === "execution_approved"`) — or including the wake reason in the `ExecutionStageWakeContext` passed to this renderer — would make the condition explicit and safe.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "feat(execution): execution_approved wake..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @@ -1261,4 +1261,79 @@ describe("issue execution policy transitions", () => { | |||
| }); | |||
There was a problem hiding this comment.
shouldResetTaskSessionForWake with new wake reason
The existing test suite in heartbeat-workspace-session.test.ts has explicit cases for execution_review_requested, execution_approval_requested, and execution_changes_requested, but no case for the newly added execution_approved reason. Adding a parallel test there would ensure consistency and guard against regressions if the function is refactored.
Prompt To Fix With AI
This is a comment left during a code review.
Path: server/src/__tests__/issue-execution-policy.test.ts
Line: 1261
Comment:
**Missing test for `shouldResetTaskSessionForWake` with new wake reason**
The existing test suite in `heartbeat-workspace-session.test.ts` has explicit cases for `execution_review_requested`, `execution_approval_requested`, and `execution_changes_requested`, but no case for the newly added `execution_approved` reason. Adding a parallel test there would ensure consistency and guard against regressions if the function is refactored.
How can I resolve this? If you propose a fix, please make it concise.| } else if (executionStage.wakeRole === "executor") { | ||
| lines.push( | ||
| "You are waking because changes were requested in the execution workflow.", | ||
| "Address the requested changes on this issue and resubmit when the work is ready.", | ||
| "", | ||
| ); | ||
| if (executionStage.lastDecisionOutcome === "approved") { | ||
| lines.push( | ||
| "Your submitted work on this issue was APPROVED and the execution workflow is complete.", | ||
| "Acknowledge the approval, wrap up any remaining handoff notes, and do not continue executor work on this issue.", | ||
| "", | ||
| ); | ||
| } else { | ||
| lines.push( | ||
| "You are waking because changes were requested in the execution workflow.", | ||
| "Address the requested changes on this issue and resubmit when the work is ready.", | ||
| "", | ||
| ); | ||
| } |
There was a problem hiding this comment.
lastDecisionOutcome alone, not tied to wake reason
The condition executionStage.lastDecisionOutcome === "approved" is used to decide which executor prompt to show, but the buildCompletedState function sets lastDecisionOutcome: "approved" as a permanent field even if the state later transitions elsewhere. If an executor receives a non-execution_approved wake while the last decision happens to be "approved" (e.g. a liveness / comment wake during a completed-but-not-yet-acknowledged window), they will see the completion prompt instead of a general-purpose one.
Tying this branch to a wake-reason check (executionStage.wakeRole === "executor" && wakeReason === "execution_approved") — or including the wake reason in the ExecutionStageWakeContext passed to this renderer — would make the condition explicit and safe.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/adapter-utils/src/server-utils.ts
Line: 691-704
Comment:
**Approved-prompt fires on `lastDecisionOutcome` alone, not tied to wake reason**
The condition `executionStage.lastDecisionOutcome === "approved"` is used to decide which executor prompt to show, but the `buildCompletedState` function sets `lastDecisionOutcome: "approved"` as a permanent field even if the state later transitions elsewhere. If an executor receives a non-`execution_approved` wake while the last decision happens to be `"approved"` (e.g. a liveness / comment wake during a completed-but-not-yet-acknowledged window), they will see the completion prompt instead of a general-purpose one.
Tying this branch to a wake-reason check (`executionStage.wakeRole === "executor" && wakeReason === "execution_approved"`) — or including the wake reason in the `ExecutionStageWakeContext` passed to this renderer — would make the condition explicit and safe.
How can I resolve this? If you propose a fix, please make it concise.
ramonmatias19
force-pushed
the
feature/execution-approved-wake
branch
from
b03d1c1 to
846eb74
Compare
- Emit an 'execution_approved' wake for the return-assignee agent when the execution workflow completes, so the executor gets a clean acknowledgement cycle instead of silently leaving the issue done - Update renderPaperclipWakePrompt to produce a distinct prompt when lastDecisionOutcome === 'approved' (tell the agent the work was accepted and to wrap up handoff notes) - Add 'execution_approved' to shouldResetTaskSessionForWake and describeSessionResetReason so the new wake type resets the task session consistently with other workflow transitions - Fix selectStageParticipant to fall back to the full participant pool when excluding the return assignee would produce an empty set — lets an executor self-approve when the execution policy lists only that participant on the stage - Add a regression test for the self-approval fallback
ramonmatias19
force-pushed
the
feature/execution-approved-wake
branch
from
846eb74 to
024032d
Compare
1 participant
Summary
execution_approvedwake for the return-assignee agent when the execution workflow completes, so the executor gets a clean acknowledgement cycle instead of silently leaving the issue donerenderPaperclipWakePromptto produce a distinct prompt whenlastDecisionOutcome === 'approved'(tell the agent the work was accepted and to wrap up handoff notes)execution_approvedtoshouldResetTaskSessionForWakeanddescribeSessionResetReasonso the new wake type resets the task session consistently with other workflow transitionsselectStageParticipantto fall back to the full participant pool when excluding the return assignee would produce an empty set — lets an executor self-approve when the execution policy lists only that participant on the stageTest plan
execution_approvedheartbeat with the new promptin_reviewwithout error