refactor(workflow): make agent call lifecycle atomic - #120
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/workflow-store.test.ts (1)
244-334: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winExtend atomic rollback coverage to
failAgentCall.This test verifies rollback for
startAgentCall,completeAgentCall, andcacheAgentCall, butfailAgentCall(workflow-store.ts lines 819-847) was refactored the same way (row update + event insert in one transaction) and has no equivalent trigger-based rollback test here.♻️ Suggested addition mirroring the existing pattern
assert.equal(store.getAgentCall(atomicRun.id, 1), undefined); atomicDb.sqlite.exec(`drop trigger reject_agent_call_cached`); + + atomicDb.sqlite.exec(` + create trigger reject_agent_call_failed + before insert on workflow_events + when new.type = 'agent_call_failed' + begin + select raise(abort, 'reject failed event'); + end; + `); + assert.throws(() => + store.failAgentCall({ + runId: atomicRun.id, + callIndex: 0, + error: "boom", + }), + ); + assert.equal(store.getAgentCall(atomicRun.id, 0)?.status, "from_cache"); + atomicDb.sqlite.exec(`drop trigger reject_agent_call_failed`); } finally { atomicDb.close(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/workflow-store.test.ts` around lines 244 - 334, Add trigger-based atomic rollback coverage for failAgentCall in the existing atomicRun test: after starting the call, create a workflow_events trigger rejecting agent_call_failed, assert failAgentCall throws, and verify getAgentCall still reports status "running"; drop the trigger, then complete the call and preserve the existing cleanup and assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/workflow-store.test.ts`:
- Around line 244-334: Add trigger-based atomic rollback coverage for
failAgentCall in the existing atomicRun test: after starting the call, create a
workflow_events trigger rejecting agent_call_failed, assert failAgentCall
throws, and verify getAgentCall still reports status "running"; drop the
trigger, then complete the call and preserve the existing cleanup and
assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: def862d1-dd83-468f-b121-abe381f368fb
📒 Files selected for processing (5)
src/workflow-api.tssrc/workflow-engine.test.tssrc/workflow-store.test.tssrc/workflow-store.tssrc/workflow-ui.test.ts
Greptile SummaryThis PR makes workflow agent-call state transitions and lifecycle events atomic.
Confidence Score: 3/5The failure-persistence path needs correction before merging because it can mask the underlying call error and leave the call indefinitely marked as running. The catch path invokes transactional failure persistence without guarding against an event-insertion error, so rollback preserves the running state and the persistence exception replaces the original provider or worktree failure. Files Needing Attention: src/workflow-api.ts and src/workflow-store.ts
|
| Filename | Overview |
|---|---|
| src/workflow-api.ts | Moves call creation ahead of worktree setup and delegates lifecycle transitions atomically, but an unhandled failure while recording a failed transition masks the original error. |
| src/workflow-store.ts | Adds SQLite transactions coupling agent-call row mutations with lifecycle events; rollback can leave a failed invocation recorded as running when failure persistence itself errors. |
| src/workflow-store.test.ts | Adds trigger-based tests confirming event-insertion failures roll back matching agent-call state changes. |
| src/workflow-engine.test.ts | Updates setup-failure expectations to require a retained failed agent-call record. |
| src/workflow-ui.test.ts | Updates the UI fixture to use the renamed call-starting API. |
Sequence Diagram
sequenceDiagram
participant API as Workflow API
participant Store as Workflow Store
participant DB as SQLite
API->>Store: startAgentCall(...)
Store->>DB: BEGIN IMMEDIATE
Store->>DB: Insert running call
Store->>DB: Insert agent_call_started
Store->>DB: COMMIT
alt call succeeds
API->>Store: completeAgentCall(...)
Store->>DB: Update call and insert completed event atomically
else call fails
API->>Store: failAgentCall(...)
Store->>DB: Update call and insert failed event atomically
else replay hit
API->>Store: cacheAgentCall(...)
Store->>DB: Insert cached call and event atomically
end
Comments Outside Diff (1)
-
src/workflow-api.ts, line 505-512 (link)Failure persistence masks call errors
When
failAgentCallcannot insert its lifecycle event, its transaction rolls back the status update and the unguarded persistence exception replaces the original provider or worktree error. The call then remains permanentlyrunningin the UI and CLI because stale-run reaping does not reconcile agent-call rows.
Reviews (1): Last reviewed commit: "test(workflow): cover atomic agent call ..." | Re-trigger Greptile
1 participant
Summary
Validation
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
beginAgentCalltostartAgentCall.