Skip to content

fix: recover the original result on a duplicate approval decision - #123

Closed
vaibhav8a wants to merge 1 commit into
extra-org:mainfrom
vaibhav8a:fix/http-duplicate-approval-recovery
Closed

fix: recover the original result on a duplicate approval decision#123
vaibhav8a wants to merge 1 commit into
extra-org:mainfrom
vaibhav8a:fix/http-duplicate-approval-recovery

Conversation

@vaibhav8a

Copy link
Copy Markdown

Closes #109.

Problem

/approve, /reject and /decision all funnel through _decide in src/agent_engine/api/app.py, which mapped ApprovalAlreadyProcessed straight to a 409 with no recovery. A client that retries after a network timeout has no way to read 409 as "the decision you're re-sending already succeeded", so a successful approval surfaces as an error.

ConversationService.resume_run already handles this exact exception by recovering the original result. The HTTP layer was the inconsistent one.

Change

Catch ApprovalAlreadyProcessed ahead of the general ApprovalError handler and recover through engine.get_processed_result(...), mirroring the agent_manager precedent:

except ApprovalAlreadyProcessed as exc:
    recovered = await engine.get_processed_result(
        run_id, approval_id,
        caller_user_id=user_id,
        caller_session_id=caller_session_id,
    )
    if recovered is None:
        raise _map_approval_error(exc) from exc
    result = recovered

The 409 is still returned when the result is genuinely unavailable, so nothing that previously failed now silently succeeds. Every other ApprovalError keeps its existing mapping, and session-scoping is unchanged — get_processed_result receives the same caller_session_id that resume did (hoisted into a local so the two calls can't drift).

Testing

Added test_duplicate_approval_recovers_the_original_result in tests/api/test_approval_endpoints.py: it approves once, approves again, and asserts the second call returns 200 with the same status and answer as the first.

Confirmed it guards the change — with the app.py hunk stashed it fails on assert 409 == 200.

pytest tests/api tests/approvals   # 134 passed
ruff check src/agent_engine/api/app.py tests/api/test_approval_endpoints.py   # All checks passed

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
@AmitAvital1

Copy link
Copy Markdown
Collaborator

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!
We will hope to see your contribution again on other issues.

For now i'm closing this PR.
Thanks

@vaibhav8a

Copy link
Copy Markdown
Author

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 closedByPullRequestsReferences on the issue catches both shapes — a merged PR that left the issue open, and an open PR whose branch name doesn't mention the issue — which is what I'd been missing. I'll come back with something that isn't already covered.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

HTTP API doesn't recover from duplicate approval decision

2 participants