fix(agents): validate phase transitions during exception recovery - #291
fix(agents): validate phase transitions during exception recovery#291erensh27 wants to merge 1 commit into
Conversation
The exception handlers in act() and observe() used set_phase(), which skips successor validation. Switch them to _set_phase() so recovery transitions are validated, with a direct reset fallback for the case where the failure interrupted a transition (e.g. inside PRE_ACT) and READY is not a valid successor -- otherwise the validation error itself would mask the original exception.
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
CLA check done |
|
I believe we attempted to incorporate a change like this in the past, but some error handling path broke as a result, so we ended up reverting it. I'm not sure if it's exactly the same change or not. But I'm a bit reticent to change this part absent a very compelling reason to do so. If you have a specific situation where this change would help in a concrete way then we could take a look, but otherwise I'm inclined to leave this as it is for now. |
Fix for #238: the exception handlers in
act()andobserve()called the publicset_phase()(which sets the phase without validating successors) instead of_set_phase()(which validates viacheck_successor), so error recovery could leave the agent in an invalid phase.Changes in
concordia/agents/entity_agent.py:excepthandlers now try_set_phase(Phase.READY)first, so recovery transitions are validated like the normal path.pre_actwhile the phase isPRE_ACT, whereREADYis not a valid successor), the validation itself would raise and mask the original exception. In that case we fall back to a directset_phase(READY)reset so the entity stays usable for other threads.No tests currently exist for
EntityAgent(verified:tests/and innertest coverage underconcordia/agents/is absent), so no regression test was added; the change is limited to the two recovery paths.