fix: recover the original result on a duplicate approval decision - #123
fix: recover the original result on a duplicate approval decision#123vaibhav8a wants to merge 1 commit into
Conversation
The /approve, /reject and /decision endpoints all funnel through _decide, which mapped ApprovalAlreadyProcessed straight to a 409. A client retrying after a network timeout has no way to read that as "the decision you are re-sending already succeeded", so a successful approval surfaced as an error. Catch ApprovalAlreadyProcessed and recover via engine.get_processed_result, exactly as ConversationService already does for the same exception, and keep the 409 only when the result is genuinely unavailable. Closes #109
|
Hi @vaibhav8a ! Thanks for contributing but this issue already was assigned to someone else, with already advance PR that merged (#110). so this issue has been close. For future things, please see inside the issue if have already open PR or if its assign to other person. this will omit the duplication working! For now i'm closing this PR. |
|
Understood, and thanks for the clear steer — #110 was merged with the issue left open, so I picked it up off an open-issue sweep without checking what had already landed against it. Entirely my miss. I've also closed my own #125 for the same reason: #86 has been open since 2 August with the same fix and a regression test. For what it's worth, checking |
Closes #109.
Problem
/approve,/rejectand/decisionall funnel through_decideinsrc/agent_engine/api/app.py, which mappedApprovalAlreadyProcessedstraight to a409with no recovery. A client that retries after a network timeout has no way to read409as "the decision you're re-sending already succeeded", so a successful approval surfaces as an error.ConversationService.resume_runalready handles this exact exception by recovering the original result. The HTTP layer was the inconsistent one.Change
Catch
ApprovalAlreadyProcessedahead of the generalApprovalErrorhandler and recover throughengine.get_processed_result(...), mirroring theagent_managerprecedent:The
409is still returned when the result is genuinely unavailable, so nothing that previously failed now silently succeeds. Every otherApprovalErrorkeeps its existing mapping, and session-scoping is unchanged —get_processed_resultreceives the samecaller_session_idthatresumedid (hoisted into a local so the two calls can't drift).Testing
Added
test_duplicate_approval_recovers_the_original_resultintests/api/test_approval_endpoints.py: it approves once, approves again, and asserts the second call returns200with the samestatusandansweras the first.Confirmed it guards the change — with the
app.pyhunk stashed it fails onassert 409 == 200.