-
-
Notifications
You must be signed in to change notification settings - Fork 40
Quarantine audit trail records a false from_status; claimable route undocumented #2345
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
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 |
|---|---|---|
|
|
@@ -2,6 +2,7 @@ | |
|
|
||
| import pytest | ||
|
|
||
| from tinyagentos.board_audit import BoardAuditLog | ||
| from tinyagentos.projects import task_store as task_store_mod | ||
| from tinyagentos.projects.task_store import ProjectTaskStore | ||
|
|
||
|
|
@@ -599,3 +600,23 @@ async def test_no_broker_no_error(tmp_path): | |
| await s.claim_task(task["id"], "worker-1") | ||
| await s.close_task(task["id"], "worker-1") | ||
| await s.close() | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_quarantine_claimed_task_records_actual_from_status(tmp_path): | ||
|
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. WARNING: New test only covers the claimed→quarantined path — no assertion for the open (unclaimed) path The test exercises the previously-buggy claimed case but never quarantines an unclaimed open task. If the Reply with |
||
| audit = BoardAuditLog(tmp_path / "audit.db") | ||
| await audit.init() | ||
| s = ProjectTaskStore(tmp_path / "tasks.db", audit=audit) | ||
| await s.init() | ||
| try: | ||
| task = await s.create_task("prj-1", "Task", "alice") | ||
| await s.claim_task(task["id"], "worker-1") | ||
| ok = await s.quarantine_task(task["id"], "system") | ||
| assert ok is True | ||
| history = await audit.history(task["id"]) | ||
| quarantined = [h for h in history if h["event"] == "task.quarantined"] | ||
| assert len(quarantined) == 1 | ||
| assert quarantined[0]["from_status"] == "claimed" | ||
| finally: | ||
| await s.close() | ||
| await audit.close() | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -446,8 +446,13 @@ async def quarantine_task(self, task_id: str, actor: str) -> bool: | |
| "task.quarantined", | ||
| {"id": task_id, "actor": actor}, | ||
| ) | ||
| # Derive the pre-quarantine status race-free from the committed row | ||
| # rather than a separate pre-read (which would have a TOCTOU gap). | ||
| # quarantine does not clear claimed_by, so a set claimer means it was | ||
| # 'claimed' (cf. close_task's derivation). | ||
| from_status = "claimed" if existing and existing.get("claimed_by") else "open" | ||
|
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. WARNING: The ternary collapses every status except Reply with |
||
| await self._record_audit( | ||
| task_id, "task.quarantined", actor, "open", "quarantined", | ||
| task_id, "task.quarantined", actor, from_status, "quarantined", | ||
| project_id=existing["project_id"] if existing else "", | ||
| ) | ||
| return changed | ||
|
|
||
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.
SUGGESTION:
claimableroute description is ambiguous about label semantics"It toggles only the
claimablelabel" could be read as replacing all labels with["claimable"]rather than adding/removing it in place. Theunquarantineentry below it avoids this ambiguity by describing its behaviour explicitly ("clears its strikes"). Consider rephrasing to: "It adds or removes theclaimablelabel while preserving every other label" to match the precision of the neighbouring bullet.Reply with
@kilocode-bot fix itto have Kilo Code address this issue.