-
Notifications
You must be signed in to change notification settings - Fork 1
feat: add approval workflow gates to TaskEngine #365
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 4 commits
70c3731
b999b65
3b5068f
0664529
91823f4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -122,6 +122,25 @@ async def save(self, item: ApprovalItem) -> ApprovalItem | None: | |||||||||
| self._items[item.id] = item | ||||||||||
| return item | ||||||||||
|
|
||||||||||
| async def save_if_pending( | ||||||||||
| self, | ||||||||||
| item: ApprovalItem, | ||||||||||
| ) -> ApprovalItem | None: | ||||||||||
| """Update only if the stored item is still PENDING. | ||||||||||
|
|
||||||||||
| Returns the saved item on success, or ``None`` if the stored | ||||||||||
| item is no longer PENDING (concurrent decision detected). | ||||||||||
| """ | ||||||||||
| current = self._items.get(item.id) | ||||||||||
| if current is None: | ||||||||||
| return None | ||||||||||
| # Apply lazy expiration check before comparing status. | ||||||||||
| current = self._check_expiration(current) | ||||||||||
| if current.status != ApprovalStatus.PENDING: | ||||||||||
| return None | ||||||||||
| self._items[item.id] = item | ||||||||||
| return item | ||||||||||
|
|
||||||||||
| def _check_expiration(self, item: ApprovalItem) -> ApprovalItem: | ||||||||||
| """Lazily expire a pending item past its ``expires_at``. | ||||||||||
|
|
||||||||||
|
|
@@ -148,6 +167,15 @@ def _check_expiration(self, item: ApprovalItem) -> ApprovalItem: | |||||||||
| approval_id=item.id, | ||||||||||
| ) | ||||||||||
| if self._on_expire is not None: | ||||||||||
| self._on_expire(expired) | ||||||||||
| try: | ||||||||||
| self._on_expire(expired) | ||||||||||
| except MemoryError, RecursionError: | ||||||||||
| raise | ||||||||||
|
Comment on lines
+172
to
+173
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Python 3
The same bug appears throughout all the new files added in this PR:
Note: the existing (pre-PR) code in Fix every occurrence by wrapping the two types in parentheses:
Suggested change
The pattern to apply globally is: # Wrong (Python 2)
except MemoryError, RecursionError:
# Correct (Python 3)
except (MemoryError, RecursionError):Prompt To Fix With AIThis is a comment left during a code review.
Path: src/ai_company/api/approval_store.py
Line: 172-173
Comment:
**Python 3 `except` tuple syntax missing parentheses — SyntaxError across all new files**
`except MemoryError, RecursionError:` is Python 2 syntax. In Python 3 the comma-separated form (without parentheses) is a `SyntaxError`; the tuple must be explicitly wrapped. Any module that contains this form will fail to import entirely.
The same bug appears throughout all the new files added in this PR:
- `src/ai_company/api/approval_store.py:172`
- `src/ai_company/api/controllers/approvals.py:100`
- `src/ai_company/engine/approval_gate.py:127, 149, 202, 216`
- `src/ai_company/engine/loop_helpers.py:69, 114, 186, 303, 387`
- `src/ai_company/tools/approval_tool.py:162, 238`
- `src/ai_company/tools/invoker.py:212, 295, 639`
Note: the existing (pre-PR) code in `invoker.py` already uses the correct form `except (MemoryError, RecursionError) as exc:` at lines 453, 539, 575, and 698 — confirming this is an inconsistency introduced only in the new additions.
Fix every occurrence by wrapping the two types in parentheses:
```suggestion
except (MemoryError, RecursionError):
raise
```
The pattern to apply globally is:
```python
# Wrong (Python 2)
except MemoryError, RecursionError:
# Correct (Python 3)
except (MemoryError, RecursionError):
```
How can I resolve this? If you propose a fix, please make it concise. |
||||||||||
| except Exception: | ||||||||||
| logger.exception( | ||||||||||
| API_APPROVAL_EXPIRED, | ||||||||||
| approval_id=item.id, | ||||||||||
| note="on_expire callback failed", | ||||||||||
| ) | ||||||||||
|
Comment on lines
+170
to
+179
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Import the project
Suggested fix from ai_company.api.errors import ConflictError
from ai_company.core.approval import ApprovalItem # noqa: TC001
from ai_company.core.enums import (
ApprovalRiskLevel,
ApprovalStatus,
)
+from ai_company.memory.errors import MemoryError as StoreMemoryError
...
- except MemoryError, RecursionError:
+ except StoreMemoryError, MemoryError, RecursionError:
raiseAs per coding guidelines, "Handle errors explicitly, never silently swallow exceptions." 🤖 Prompt for AI Agents |
||||||||||
| return expired | ||||||||||
| return item | ||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick | 🔵 Trivial
Consider adding comma after "e.g." for consistency.
Static analysis flagged the missing comma. While minor, it improves readability in the dense event constant listing.
🧰 Tools
🪛 LanguageTool
[style] ~154-~154: A comma is missing here.
Context: ...nder
ai_company.observability.events(e.g.PROVIDER_CALL_STARTfrom `events.prov...(EG_NO_COMMA)
🤖 Prompt for AI Agents