-
Notifications
You must be signed in to change notification settings - Fork 0
feat: add approval workflow gates to TaskEngine #387
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 all commits
2453285
5c08597
d4c62c5
2454b2a
90877fd
59fb102
e6ff984
24e53df
228fedd
1d0166b
acd6e17
c794c11
cc65e53
d71ab67
58dcc2a
9b0638e
9b97a4a
b36b9ca
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,32 @@ 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: | ||
| """Conditionally update an approval item if it is still pending. | ||
|
|
||
| A lazy expiration check is applied before comparing status. | ||
|
|
||
| Returns: | ||
| The saved item on success, or ``None`` if: | ||
|
|
||
| * no item with the given ID exists in the store, | ||
| * the stored item has expired, or | ||
| * the stored item is no longer ``PENDING`` (e.g. a | ||
| concurrent decision was made). | ||
| """ | ||
| 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 +174,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: | ||
|
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. |
||
| raise | ||
| except Exception: | ||
| logger.exception( | ||
| API_APPROVAL_EXPIRED, | ||
| approval_id=item.id, | ||
| note="on_expire callback failed", | ||
| ) | ||
|
Comment on lines
+178
to
+186
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. Lazy expiration not written back to When If Consider writing back the expired record when expiration fires: current = self._check_expiration(current)
if current.status != ApprovalStatus.PENDING:
# Write back the expired state so subsequent reads are consistent.
self._items[item.id] = current
return NonePrompt To Fix With AIThis is a comment left during a code review.
Path: src/ai_company/api/approval_store.py
Line: 178-186
Comment:
**Lazy expiration not written back to `_items` inside `save_if_pending`**
When `_check_expiration` determines the stored item has expired, it returns an expired copy but does **not** update `self._items`. After `save_if_pending` returns `None`, `self._items[item.id]` still holds the original `PENDING` item. Any subsequent call to `get()` (or another `save_if_pending`) would retrieve the stale PENDING entry and re-run the expiry check again, but the expired `on_expire` callback would fire a second time.
If `get()` also applies lazy expiration this is harmless. If it doesn't, callers that check the status after a failed `save_if_pending` could observe an inconsistent `PENDING` state.
Consider writing back the expired record when expiration fires:
```python
current = self._check_expiration(current)
if current.status != ApprovalStatus.PENDING:
# Write back the expired state so subsequent reads are consistent.
self._items[item.id] = current
return None
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+177
to
+186
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. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Check if MemoryError is imported or shadowed in this file
rg -n "import.*MemoryError|from.*import.*MemoryError" src/ai_company/api/approval_store.py
# Check if there's a pattern of catching built-in MemoryError elsewhere
rg -nC2 "except.*MemoryError" src/ai_company --type py | head -40Repository: Aureliolo/synthorg Length of output: 2521 🏁 Script executed: #!/bin/bash
# Get the imports at the top of approval_store.py
head -50 src/ai_company/api/approval_store.py
# Check if ai_company.memory.errors is imported anywhere
rg "from ai_company.memory.errors|import.*MemoryError" src/ai_company/api/approval_store.py
# Get the full context of the method containing the suspect code
sed -n '160,190p' src/ai_company/api/approval_store.pyRepository: Aureliolo/synthorg Length of output: 2396 🏁 Script executed: #!/bin/bash
# Verify if ai_company.memory.errors.MemoryError exists in codebase
find src -name "*.py" -exec grep -l "class MemoryError" {} \;
# Check ai_company.memory.errors module
fd "memory" src/ai_company --type dRepository: Aureliolo/synthorg Length of output: 116 🏁 Script executed: #!/bin/bash
# Check if MemoryError is actually defined in ai_company.memory.errors
cat src/ai_company/memory/errors.pyRepository: Aureliolo/synthorg Length of output: 1471 🏁 Script executed: #!/bin/bash
# Check if approval_store.py imports from ai_company.memory.errors
rg "from ai_company.memory|import.*MemoryError" src/ai_company/api/approval_store.py
# Check the complete import section of approval_store.py
sed -n '1,30p' src/ai_company/api/approval_store.pyRepository: Aureliolo/synthorg Length of output: 877 Add logging before re-raising The exception handler re-raises Log before the re-raise on line 179: except MemoryError, RecursionError:
logger.error(
API_APPROVAL_EXPIRED,
approval_id=item.id,
note="on_expire callback failed - non-recoverable error",
)
raise🤖 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.
Fix the punctuation typo in the logging guidance line.
Line 154 has a comma omission in the long sentence (as flagged by LanguageTool). Please add the missing comma to keep docs lint/style clean.
🧰 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