fix(api): recover the original result on a retried approval decision

[Rsakhuja]

Fixes#109
Catches ApprovalAlreadyProcessed exception and recovers the original result in order to avoid returning a 409 error when a duplicate request is made.

[Asaf-prog]

Thanks for the fix — the overall approach looks correct, and recovering the already-processed result is consistent with the existing agent_manager behavior.

One issue I think we should address before merging:

If engine.get_processed_result() raises an unexpected exception that is not an ApprovalError (for example, a checkpointer/database/runtime failure), it will not be caught by the outer except Exception block, because it is raised from inside the except ApprovalAlreadyProcessed handler.

That means the recovery path currently bypasses the API's existing error sanitization behavior.

I suggest adding an except Exception around get_processed_result() that logs the failure and returns the standard sanitized 500 Internal server error.

It would also be good to add a test where get_processed_result() raises something like RuntimeError("private recovery failure") and verify that:

• the response is 500
• the response only contains Internal server error
• the internal exception message is not exposed to the client

Other than that, the change looks good to me.

[Rsakhuja]

Thanks for the fix — the overall approach looks correct, and recovering the already-processed result is consistent with the existing agent_manager behavior.

One issue I think we should address before merging:

If engine.get_processed_result() raises an unexpected exception that is not an ApprovalError (for example, a checkpointer/database/runtime failure), it will not be caught by the outer except Exception block, because it is raised from inside the except ApprovalAlreadyProcessed handler.

That means the recovery path currently bypasses the API's existing error sanitization behavior.

I suggest adding an except Exception around get_processed_result() that logs the failure and returns the standard sanitized 500 Internal server error.

It would also be good to add a test where get_processed_result() raises something like RuntimeError("private recovery failure") and verify that:

• the response is 500
• the response only contains Internal server error
• the internal exception message is not exposed to the client

Other than that, the change looks good to me.

Hi. That makes sense. I updated the PR to include the except Exception and the additional test.

[Rsakhuja]

Fixes#109
Catches ApprovalAlreadyProcessed exception and recovers the original result in order to avoid returning a 409 error when a duplicate request is made.

[Asaf-prog]

Thanks for the fix — the overall approach looks correct, and recovering the already-processed result is consistent with the existing agent_manager behavior.

One issue I think we should address before merging:

If engine.get_processed_result() raises an unexpected exception that is not an ApprovalError (for example, a checkpointer/database/runtime failure), it will not be caught by the outer except Exception block, because it is raised from inside the except ApprovalAlreadyProcessed handler.

That means the recovery path currently bypasses the API's existing error sanitization behavior.

I suggest adding an except Exception around get_processed_result() that logs the failure and returns the standard sanitized 500 Internal server error.

It would also be good to add a test where get_processed_result() raises something like RuntimeError("private recovery failure") and verify that:

• the response is 500
• the response only contains Internal server error
• the internal exception message is not exposed to the client

Other than that, the change looks good to me.

[Rsakhuja]

Thanks for the fix — the overall approach looks correct, and recovering the already-processed result is consistent with the existing agent_manager behavior.

One issue I think we should address before merging:

If engine.get_processed_result() raises an unexpected exception that is not an ApprovalError (for example, a checkpointer/database/runtime failure), it will not be caught by the outer except Exception block, because it is raised from inside the except ApprovalAlreadyProcessed handler.

That means the recovery path currently bypasses the API's existing error sanitization behavior.

I suggest adding an except Exception around get_processed_result() that logs the failure and returns the standard sanitized 500 Internal server error.

It would also be good to add a test where get_processed_result() raises something like RuntimeError("private recovery failure") and verify that:

• the response is 500
• the response only contains Internal server error
• the internal exception message is not exposed to the client

Other than that, the change looks good to me.

Hi. That makes sense. I updated the PR to include the except Exception and the additional test.

[Rsakhuja]

Fixes#109
Catches ApprovalAlreadyProcessed exception and recovers the original result in order to avoid returning a 409 error when a duplicate request is made.

[Asaf-prog]

Thanks for the fix — the overall approach looks correct, and recovering the already-processed result is consistent with the existing agent_manager behavior.

One issue I think we should address before merging:

If engine.get_processed_result() raises an unexpected exception that is not an ApprovalError (for example, a checkpointer/database/runtime failure), it will not be caught by the outer except Exception block, because it is raised from inside the except ApprovalAlreadyProcessed handler.

That means the recovery path currently bypasses the API's existing error sanitization behavior.

I suggest adding an except Exception around get_processed_result() that logs the failure and returns the standard sanitized 500 Internal server error.

It would also be good to add a test where get_processed_result() raises something like RuntimeError("private recovery failure") and verify that:

• the response is 500
• the response only contains Internal server error
• the internal exception message is not exposed to the client

Other than that, the change looks good to me.

[Rsakhuja]

Thanks for the fix — the overall approach looks correct, and recovering the already-processed result is consistent with the existing agent_manager behavior.

One issue I think we should address before merging:

If engine.get_processed_result() raises an unexpected exception that is not an ApprovalError (for example, a checkpointer/database/runtime failure), it will not be caught by the outer except Exception block, because it is raised from inside the except ApprovalAlreadyProcessed handler.

That means the recovery path currently bypasses the API's existing error sanitization behavior.

I suggest adding an except Exception around get_processed_result() that logs the failure and returns the standard sanitized 500 Internal server error.

It would also be good to add a test where get_processed_result() raises something like RuntimeError("private recovery failure") and verify that:

• the response is 500
• the response only contains Internal server error
• the internal exception message is not exposed to the client

Other than that, the change looks good to me.

Hi. That makes sense. I updated the PR to include the except Exception and the additional test.

[Rsakhuja]

Fixes#109
Catches ApprovalAlreadyProcessed exception and recovers the original result in order to avoid returning a 409 error when a duplicate request is made.

[Asaf-prog]

Thanks for the fix — the overall approach looks correct, and recovering the already-processed result is consistent with the existing agent_manager behavior.

One issue I think we should address before merging:

If engine.get_processed_result() raises an unexpected exception that is not an ApprovalError (for example, a checkpointer/database/runtime failure), it will not be caught by the outer except Exception block, because it is raised from inside the except ApprovalAlreadyProcessed handler.

That means the recovery path currently bypasses the API's existing error sanitization behavior.

I suggest adding an except Exception around get_processed_result() that logs the failure and returns the standard sanitized 500 Internal server error.

It would also be good to add a test where get_processed_result() raises something like RuntimeError("private recovery failure") and verify that:

• the response is 500
• the response only contains Internal server error
• the internal exception message is not exposed to the client

Other than that, the change looks good to me.

[Rsakhuja]

Thanks for the fix — the overall approach looks correct, and recovering the already-processed result is consistent with the existing agent_manager behavior.

One issue I think we should address before merging:

If engine.get_processed_result() raises an unexpected exception that is not an ApprovalError (for example, a checkpointer/database/runtime failure), it will not be caught by the outer except Exception block, because it is raised from inside the except ApprovalAlreadyProcessed handler.

That means the recovery path currently bypasses the API's existing error sanitization behavior.

I suggest adding an except Exception around get_processed_result() that logs the failure and returns the standard sanitized 500 Internal server error.

It would also be good to add a test where get_processed_result() raises something like RuntimeError("private recovery failure") and verify that:

• the response is 500
• the response only contains Internal server error
• the internal exception message is not exposed to the client

Other than that, the change looks good to me.

Hi. That makes sense. I updated the PR to include the except Exception and the additional test.

[Rsakhuja]

Fixes#109
Catches ApprovalAlreadyProcessed exception and recovers the original result in order to avoid returning a 409 error when a duplicate request is made.

[Asaf-prog]

Thanks for the fix — the overall approach looks correct, and recovering the already-processed result is consistent with the existing agent_manager behavior.

One issue I think we should address before merging:

If engine.get_processed_result() raises an unexpected exception that is not an ApprovalError (for example, a checkpointer/database/runtime failure), it will not be caught by the outer except Exception block, because it is raised from inside the except ApprovalAlreadyProcessed handler.

That means the recovery path currently bypasses the API's existing error sanitization behavior.

I suggest adding an except Exception around get_processed_result() that logs the failure and returns the standard sanitized 500 Internal server error.

It would also be good to add a test where get_processed_result() raises something like RuntimeError("private recovery failure") and verify that:

• the response is 500
• the response only contains Internal server error
• the internal exception message is not exposed to the client

Other than that, the change looks good to me.

[Rsakhuja]

Thanks for the fix — the overall approach looks correct, and recovering the already-processed result is consistent with the existing agent_manager behavior.

One issue I think we should address before merging:

If engine.get_processed_result() raises an unexpected exception that is not an ApprovalError (for example, a checkpointer/database/runtime failure), it will not be caught by the outer except Exception block, because it is raised from inside the except ApprovalAlreadyProcessed handler.

That means the recovery path currently bypasses the API's existing error sanitization behavior.

I suggest adding an except Exception around get_processed_result() that logs the failure and returns the standard sanitized 500 Internal server error.

It would also be good to add a test where get_processed_result() raises something like RuntimeError("private recovery failure") and verify that:

• the response is 500
• the response only contains Internal server error
• the internal exception message is not exposed to the client

Other than that, the change looks good to me.

Hi. That makes sense. I updated the PR to include the except Exception and the additional test.

[Rsakhuja]

Fixes#109
Catches ApprovalAlreadyProcessed exception and recovers the original result in order to avoid returning a 409 error when a duplicate request is made.

[Asaf-prog]

Thanks for the fix — the overall approach looks correct, and recovering the already-processed result is consistent with the existing agent_manager behavior.

One issue I think we should address before merging:

If engine.get_processed_result() raises an unexpected exception that is not an ApprovalError (for example, a checkpointer/database/runtime failure), it will not be caught by the outer except Exception block, because it is raised from inside the except ApprovalAlreadyProcessed handler.

That means the recovery path currently bypasses the API's existing error sanitization behavior.

I suggest adding an except Exception around get_processed_result() that logs the failure and returns the standard sanitized 500 Internal server error.

It would also be good to add a test where get_processed_result() raises something like RuntimeError("private recovery failure") and verify that:

• the response is 500
• the response only contains Internal server error
• the internal exception message is not exposed to the client

Other than that, the change looks good to me.

[Rsakhuja]

Thanks for the fix — the overall approach looks correct, and recovering the already-processed result is consistent with the existing agent_manager behavior.

One issue I think we should address before merging:

If engine.get_processed_result() raises an unexpected exception that is not an ApprovalError (for example, a checkpointer/database/runtime failure), it will not be caught by the outer except Exception block, because it is raised from inside the except ApprovalAlreadyProcessed handler.

That means the recovery path currently bypasses the API's existing error sanitization behavior.

I suggest adding an except Exception around get_processed_result() that logs the failure and returns the standard sanitized 500 Internal server error.

It would also be good to add a test where get_processed_result() raises something like RuntimeError("private recovery failure") and verify that:

• the response is 500
• the response only contains Internal server error
• the internal exception message is not exposed to the client

Other than that, the change looks good to me.

Hi. That makes sense. I updated the PR to include the except Exception and the additional test.

[Rsakhuja]

Fixes#109
Catches ApprovalAlreadyProcessed exception and recovers the original result in order to avoid returning a 409 error when a duplicate request is made.

[Asaf-prog]

Thanks for the fix — the overall approach looks correct, and recovering the already-processed result is consistent with the existing agent_manager behavior.

One issue I think we should address before merging:

If engine.get_processed_result() raises an unexpected exception that is not an ApprovalError (for example, a checkpointer/database/runtime failure), it will not be caught by the outer except Exception block, because it is raised from inside the except ApprovalAlreadyProcessed handler.

That means the recovery path currently bypasses the API's existing error sanitization behavior.

I suggest adding an except Exception around get_processed_result() that logs the failure and returns the standard sanitized 500 Internal server error.

It would also be good to add a test where get_processed_result() raises something like RuntimeError("private recovery failure") and verify that:

• the response is 500
• the response only contains Internal server error
• the internal exception message is not exposed to the client

Other than that, the change looks good to me.

[Rsakhuja]

Thanks for the fix — the overall approach looks correct, and recovering the already-processed result is consistent with the existing agent_manager behavior.

One issue I think we should address before merging:

If engine.get_processed_result() raises an unexpected exception that is not an ApprovalError (for example, a checkpointer/database/runtime failure), it will not be caught by the outer except Exception block, because it is raised from inside the except ApprovalAlreadyProcessed handler.

That means the recovery path currently bypasses the API's existing error sanitization behavior.

I suggest adding an except Exception around get_processed_result() that logs the failure and returns the standard sanitized 500 Internal server error.

It would also be good to add a test where get_processed_result() raises something like RuntimeError("private recovery failure") and verify that:

• the response is 500
• the response only contains Internal server error
• the internal exception message is not exposed to the client

Other than that, the change looks good to me.

Hi. That makes sense. I updated the PR to include the except Exception and the additional test.

[Rsakhuja]

Fixes#109
Catches ApprovalAlreadyProcessed exception and recovers the original result in order to avoid returning a 409 error when a duplicate request is made.

[Asaf-prog]

Thanks for the fix — the overall approach looks correct, and recovering the already-processed result is consistent with the existing agent_manager behavior.

One issue I think we should address before merging:

If engine.get_processed_result() raises an unexpected exception that is not an ApprovalError (for example, a checkpointer/database/runtime failure), it will not be caught by the outer except Exception block, because it is raised from inside the except ApprovalAlreadyProcessed handler.

That means the recovery path currently bypasses the API's existing error sanitization behavior.

I suggest adding an except Exception around get_processed_result() that logs the failure and returns the standard sanitized 500 Internal server error.

It would also be good to add a test where get_processed_result() raises something like RuntimeError("private recovery failure") and verify that:

• the response is 500
• the response only contains Internal server error
• the internal exception message is not exposed to the client

Other than that, the change looks good to me.

[Rsakhuja]

Thanks for the fix — the overall approach looks correct, and recovering the already-processed result is consistent with the existing agent_manager behavior.

One issue I think we should address before merging:

If engine.get_processed_result() raises an unexpected exception that is not an ApprovalError (for example, a checkpointer/database/runtime failure), it will not be caught by the outer except Exception block, because it is raised from inside the except ApprovalAlreadyProcessed handler.

That means the recovery path currently bypasses the API's existing error sanitization behavior.

I suggest adding an except Exception around get_processed_result() that logs the failure and returns the standard sanitized 500 Internal server error.

It would also be good to add a test where get_processed_result() raises something like RuntimeError("private recovery failure") and verify that:

• the response is 500
• the response only contains Internal server error
• the internal exception message is not exposed to the client

Other than that, the change looks good to me.

Hi. That makes sense. I updated the PR to include the except Exception and the additional test.