fix(hooks): block to_in_progress for completed tasks (fixes #218)#491
Open
groot-guo wants to merge 1 commit into
Open
fix(hooks): block to_in_progress for completed tasks (fixes #218)#491groot-guo wants to merge 1 commit into
groot-guo wants to merge 1 commit into
Conversation
After a completion hook event (TaskComplete, stop, subagentstop, afteragent) triggers to_review with reviewReason=hook, the auto-commit PreToolUse hook fires to_in_progress which incorrectly resumes the task. Both used reviewReason=hook, so the old guard could not tell apart ask_followup_question (should resume) from completion events (should NOT resume). Add a guard that checks latestHookActivity.hookEventName against a set of completion hook event names. Block to_in_progress for these; allow for PreToolUse (ask_followup_question), null (backward compat), attention, and error review reasons.
Greptile SummaryThis PR fixes a bug where a completed task could be incorrectly flipped back to
Confidence Score: 4/5The fix correctly addresses the specific auto-commit regression; the only concern is that the guard reads from a mutable field that later activity events can overwrite. The fix works well for the described scenario. The fragility is that applyHookActivity merges string fields from any subsequent hook event, meaning an intermediate activity or blocked to_in_progress carrying a non-completion hookEventName in its metadata would silently overwrite the field and let the next to_in_progress through. This is a latent issue rather than a currently broken path. src/trpc/hooks-api.ts — specifically the interaction between the new guard and applyHookActivity in the blocked path
|
| Filename | Overview |
|---|---|
| src/trpc/hooks-api.ts | Adds COMPLETION_HOOK_EVENT_NAMES set and refactors canTransitionTaskForHookEvent to block to_in_progress when latestHookActivity.hookEventName is a completion event; guard state is mutable via applyHookActivity from subsequent hook calls |
| test/runtime/trpc/hooks-api.test.ts | Adds comprehensive tests for the new guard covering all four completion event names, PreToolUse, null backward-compat, attention, and error; missing coverage for exit and interrupted review reasons |
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
src/trpc/hooks-api.ts:44-53
**Guard relies on mutable `latestHookActivity`**
The `canTransitionTaskForHookEvent` guard reads `latestHookActivity.hookEventName` to distinguish completion hooks from PreToolUse hooks, but `applyHookActivity` merges any string-valued `hookEventName` from subsequent events into that same field (line 842–843 of `session-manager.ts`). Any blocked `to_in_progress` or `activity` event that arrives with metadata containing a `hookEventName` outside `COMPLETION_HOOK_EVENT_NAMES` will silently overwrite the field, causing the next `to_in_progress` to pass the guard even on a completed task. A safer approach is to store the hook event name that triggered the review as a dedicated, write-once field (e.g. `reviewHookEventName`) so it cannot be clobbered by later activity updates.
### Issue 2 of 2
test/runtime/trpc/hooks-api.test.ts:169-257
**Missing tests for `exit` and `interrupted` review reasons**
The new test block covers `attention`, `error`, and `hook`, but `reviewReason` can also be `"exit"` or `"interrupted"` (per `runtimeTaskSessionReviewReasonSchema`). Both fall through all the `if` branches in the new code and return `false`, matching the old behavior. Tests for these cases would lock in that expectation and prevent a future contributor from accidentally adding them to the allow-list.
Reviews (1): Last reviewed commit: "fix(hooks): block to_in_progress for com..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+44
to
+53
| // Hook-triggered reviews: disallow to_in_progress when the hook that | ||
| // triggered review was a completion event (TaskComplete, stop, etc.). | ||
| // This prevents auto-commit PreToolUse from flipping completed tasks back to in_progress. | ||
| if (summary.reviewReason === "hook") { | ||
| const hookEventName = summary.latestHookActivity?.hookEventName ?? null; | ||
| if (hookEventName && COMPLETION_HOOK_EVENT_NAMES.has(hookEventName)) { | ||
| return false; | ||
| } | ||
| return true; | ||
| } |
There was a problem hiding this comment.
latestHookActivity
The canTransitionTaskForHookEvent guard reads latestHookActivity.hookEventName to distinguish completion hooks from PreToolUse hooks, but applyHookActivity merges any string-valued hookEventName from subsequent events into that same field (line 842–843 of session-manager.ts). Any blocked to_in_progress or activity event that arrives with metadata containing a hookEventName outside COMPLETION_HOOK_EVENT_NAMES will silently overwrite the field, causing the next to_in_progress to pass the guard even on a completed task. A safer approach is to store the hook event name that triggered the review as a dedicated, write-once field (e.g. reviewHookEventName) so it cannot be clobbered by later activity updates.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/trpc/hooks-api.ts
Line: 44-53
Comment:
**Guard relies on mutable `latestHookActivity`**
The `canTransitionTaskForHookEvent` guard reads `latestHookActivity.hookEventName` to distinguish completion hooks from PreToolUse hooks, but `applyHookActivity` merges any string-valued `hookEventName` from subsequent events into that same field (line 842–843 of `session-manager.ts`). Any blocked `to_in_progress` or `activity` event that arrives with metadata containing a `hookEventName` outside `COMPLETION_HOOK_EVENT_NAMES` will silently overwrite the field, causing the next `to_in_progress` to pass the guard even on a completed task. A safer approach is to store the hook event name that triggered the review as a dedicated, write-once field (e.g. `reviewHookEventName`) so it cannot be clobbered by later activity updates.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
169
to
257
| ref: "refs/kanban/checkpoints/task-1/turn/1", | ||
| }); | ||
| }); | ||
|
|
||
| describe("to_in_progress guard for hook-triggered reviews", () => { | ||
| function makeManager(summary: RuntimeTaskSessionSummary) { | ||
| return { | ||
| getSummary: vi.fn(() => summary), | ||
| transitionToReview: vi.fn(), | ||
| transitionToRunning: vi.fn(() => summary), | ||
| applyHookActivity: vi.fn(), | ||
| } as unknown as TerminalSessionManager; | ||
| } | ||
|
|
||
| async function ingestToInProgress(summary: RuntimeTaskSessionSummary) { | ||
| const manager = makeManager(summary); | ||
| const api = createHooksApi({ | ||
| getWorkspacePathById: vi.fn(() => "/tmp/repo"), | ||
| ensureTerminalManagerForWorkspace: vi.fn(async () => manager), | ||
| broadcastRuntimeWorkspaceStateUpdated: vi.fn(), | ||
| broadcastTaskReadyForReview: vi.fn(), | ||
| }); | ||
|
|
||
| const response = await api.ingest({ | ||
| taskId: "task-1", | ||
| workspaceId: "workspace-1", | ||
| event: "to_in_progress", | ||
| }); | ||
|
|
||
| return { response, manager }; | ||
| } | ||
|
|
||
| it("blocks to_in_progress when reviewReason=hook and latestHookActivity.hookEventName=TaskComplete", async () => { | ||
| const summary = createAwaitingReviewSummary("hook", "TaskComplete"); | ||
| const { response, manager } = await ingestToInProgress(summary); | ||
| expect(response).toEqual({ ok: true }); | ||
| expect(manager.transitionToRunning).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it("blocks to_in_progress when reviewReason=hook and latestHookActivity.hookEventName=stop", async () => { | ||
| const summary = createAwaitingReviewSummary("hook", "stop"); | ||
| const { response, manager } = await ingestToInProgress(summary); | ||
| expect(response).toEqual({ ok: true }); | ||
| expect(manager.transitionToRunning).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it("blocks to_in_progress when reviewReason=hook and latestHookActivity.hookEventName=afteragent", async () => { | ||
| const summary = createAwaitingReviewSummary("hook", "afteragent"); | ||
| const { response, manager } = await ingestToInProgress(summary); | ||
| expect(response).toEqual({ ok: true }); | ||
| expect(manager.transitionToRunning).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it("blocks to_in_progress when reviewReason=hook and latestHookActivity.hookEventName=subagentstop", async () => { | ||
| const summary = createAwaitingReviewSummary("hook", "subagentstop"); | ||
| const { response, manager } = await ingestToInProgress(summary); | ||
| expect(response).toEqual({ ok: true }); | ||
| expect(manager.transitionToRunning).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it("allows to_in_progress when reviewReason=hook and latestHookActivity.hookEventName=PreToolUse (ask_followup_question)", async () => { | ||
| const summary = createAwaitingReviewSummary("hook", "PreToolUse"); | ||
| const { response, manager } = await ingestToInProgress(summary); | ||
| expect(response).toEqual({ ok: true }); | ||
| expect(manager.transitionToRunning).toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it("allows to_in_progress when reviewReason=hook and latestHookActivity=null (backward compat)", async () => { | ||
| const summary = createAwaitingReviewSummary("hook", null); | ||
| const { response, manager } = await ingestToInProgress(summary); | ||
| expect(response).toEqual({ ok: true }); | ||
| expect(manager.transitionToRunning).toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it("allows to_in_progress when reviewReason=attention (user returned)", async () => { | ||
| const summary = createAwaitingReviewSummary("attention", null); | ||
| const { response, manager } = await ingestToInProgress(summary); | ||
| expect(response).toEqual({ ok: true }); | ||
| expect(manager.transitionToRunning).toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it("allows to_in_progress when reviewReason=error (error recovery)", async () => { | ||
| const summary = createAwaitingReviewSummary("error", null); | ||
| const { response, manager } = await ingestToInProgress(summary); | ||
| expect(response).toEqual({ ok: true }); | ||
| expect(manager.transitionToRunning).toHaveBeenCalled(); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
exit and interrupted review reasons
The new test block covers attention, error, and hook, but reviewReason can also be "exit" or "interrupted" (per runtimeTaskSessionReviewReasonSchema). Both fall through all the if branches in the new code and return false, matching the old behavior. Tests for these cases would lock in that expectation and prevent a future contributor from accidentally adding them to the allow-list.
Prompt To Fix With AI
This is a comment left during a code review.
Path: test/runtime/trpc/hooks-api.test.ts
Line: 169-257
Comment:
**Missing tests for `exit` and `interrupted` review reasons**
The new test block covers `attention`, `error`, and `hook`, but `reviewReason` can also be `"exit"` or `"interrupted"` (per `runtimeTaskSessionReviewReasonSchema`). Both fall through all the `if` branches in the new code and return `false`, matching the old behavior. Tests for these cases would lock in that expectation and prevent a future contributor from accidentally adding them to the allow-list.
How can I resolve this? If you propose a fix, please make it concise.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
After a completion hook event (TaskComplete, stop, subagentstop, afteragent) triggers to_review with reviewReason=hook, the auto-commit PreToolUse hook fires to_in_progress which incorrectly resumes the task. Both used reviewReason=hook, so the old guard could not tell apart ask_followup_question (should resume) from completion events (should NOT resume).
Add a guard that checks latestHookActivity.hookEventName against a set of completion hook event names. Block to_in_progress for these; allow for PreToolUse (ask_followup_question), null (backward compat), attention, and error review reasons.