fix: recover the original result on a duplicate approval decision

[vaibhav8a]

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:

exceptApprovalAlreadyProcessedasexc:
recovered=awaitengine.get_processed_result(
run_id, approval_id,
caller_user_id=user_id,
caller_session_id=caller_session_id,
)
ifrecoveredisNone:
raise_map_approval_error(exc) fromexcresult=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

[AmitAvital1]

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]

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.

[vaibhav8a]

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:

exceptApprovalAlreadyProcessedasexc:
recovered=awaitengine.get_processed_result(
run_id, approval_id,
caller_user_id=user_id,
caller_session_id=caller_session_id,
)
ifrecoveredisNone:
raise_map_approval_error(exc) fromexcresult=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

[AmitAvital1]

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]

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.

[vaibhav8a]

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:

exceptApprovalAlreadyProcessedasexc:
recovered=awaitengine.get_processed_result(
run_id, approval_id,
caller_user_id=user_id,
caller_session_id=caller_session_id,
)
ifrecoveredisNone:
raise_map_approval_error(exc) fromexcresult=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

[AmitAvital1]

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]

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.

[vaibhav8a]

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:

exceptApprovalAlreadyProcessedasexc:
recovered=awaitengine.get_processed_result(
run_id, approval_id,
caller_user_id=user_id,
caller_session_id=caller_session_id,
)
ifrecoveredisNone:
raise_map_approval_error(exc) fromexcresult=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

[AmitAvital1]

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]

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.

[vaibhav8a]

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:

exceptApprovalAlreadyProcessedasexc:
recovered=awaitengine.get_processed_result(
run_id, approval_id,
caller_user_id=user_id,
caller_session_id=caller_session_id,
)
ifrecoveredisNone:
raise_map_approval_error(exc) fromexcresult=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

[AmitAvital1]

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]

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.

[vaibhav8a]

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:

exceptApprovalAlreadyProcessedasexc:
recovered=awaitengine.get_processed_result(
run_id, approval_id,
caller_user_id=user_id,
caller_session_id=caller_session_id,
)
ifrecoveredisNone:
raise_map_approval_error(exc) fromexcresult=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

[AmitAvital1]

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]

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.

[vaibhav8a]

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:

exceptApprovalAlreadyProcessedasexc:
recovered=awaitengine.get_processed_result(
run_id, approval_id,
caller_user_id=user_id,
caller_session_id=caller_session_id,
)
ifrecoveredisNone:
raise_map_approval_error(exc) fromexcresult=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

[AmitAvital1]

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]

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.

[vaibhav8a]

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:

exceptApprovalAlreadyProcessedasexc:
recovered=awaitengine.get_processed_result(
run_id, approval_id,
caller_user_id=user_id,
caller_session_id=caller_session_id,
)
ifrecoveredisNone:
raise_map_approval_error(exc) fromexcresult=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

[AmitAvital1]

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]

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.