feat: add explicit HITL decision statuses#2
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a standardized DecisionStatus literal and adds a status field to both ConfirmationUIResult and ToolExecutionDecision to improve the clarity of Human-In-The-Loop (HITL) decisions. The changes include logic to map legacy actions to the new status values and ensure backward compatibility when status fields are missing. Review feedback suggests enhancing the error message for unsupported confirmation actions to aid debugging and improving the robustness of the resolved_status method in ToolExecutionDecision by providing a fallback to the inference logic.
| raise ValueError( | ||
| "Unsupported confirmation action. Provide one of 'confirm', 'modify', 'reject' or set 'status' explicitly." | ||
| ) |
There was a problem hiding this comment.
The error message could be more helpful by including the actual unsupported action and listing the valid values for the status field. This assists developers in debugging custom ConfirmationUI implementations.
| raise ValueError( | |
| "Unsupported confirmation action. Provide one of 'confirm', 'modify', 'reject' or set 'status' explicitly." | |
| ) | |
| raise ValueError( | |
| f"Unsupported confirmation action '{self.action}'. " | |
| "Provide one of 'confirm', 'modify', 'reject' or set 'status' to one of " | |
| "'approved', 'modified', 'rejected' explicitly." | |
| ) |
| def resolved_status(self) -> DecisionStatus: | ||
| """ | ||
| Return the normalized decision status for this execution decision. | ||
| """ | ||
| return self.status |
There was a problem hiding this comment.
The resolved_status method currently returns self.status directly. Since self.status is typed as DecisionStatus | None, this could return None if the object was mutated (e.g., via dataclasses.replace) or if __post_init__ was bypassed. To ensure type safety and robustness, it's better to fall back to _infer_status() if self.status is missing.
| def resolved_status(self) -> DecisionStatus: | |
| """ | |
| Return the normalized decision status for this execution decision. | |
| """ | |
| return self.status | |
| def resolved_status(self) -> DecisionStatus: | |
| """ | |
| Return the normalized decision status for this execution decision. | |
| """ | |
| return self.status or self._infer_status() |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 35656e28f1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if self.status is None: | ||
| self.status = self._infer_status() | ||
| else: | ||
| self.execute = self.status != "rejected" |
There was a problem hiding this comment.
When a ToolExecutionDecision is rebuilt from an external or serialized dict and status is misspelled or from a newer value, for example {'execute': False, 'status': 'reject'}, this branch treats anything other than exactly "rejected" as executable and overwrites execute to True. That fail-open path can run a tool that the caller explicitly marked as not executable, so unknown statuses should be rejected or fall back to the legacy execute value instead of approving execution.
Useful? React with 👍 / 👎.
1 participant
Summary
approved/modified/rejectedstatuses toToolExecutionDecisionConfirmationUIResultactions into explicit HITL decision statusesexecuteboolean and infer status for older serialized payloadsCompatibility
executefieldToolExecutionDecision.from_dict()working for payloads withoutstatusNot included
Validation
.venv\\Scripts\\python.exe -m pytest test/human_in_the_loop/test_dataclasses.py test/human_in_the_loop/test_policies.py test/human_in_the_loop/test_strategies.py -k "not asyncio and not async".venv\\Scripts\\python.exe -m pytest test/components/agents/test_agent_hitl.py -k "to_dict or from_dict".venv\\Scripts\\ruff.exe check haystack/human_in_the_loop/dataclasses.py haystack/human_in_the_loop/policies.py haystack/human_in_the_loop/strategies.py test/human_in_the_loop/test_dataclasses.py.venv\\Scripts\\python.exebecausepytest-asynciois not active in this venv