tsk-glxi4e [OPEN] Make 3-strike card QUARANTINE loud (it currently v - #2172
tsk-glxi4e [OPEN] Make 3-strike card QUARANTINE loud (it currently v#2172jaylfc wants to merge 1 commit into
Conversation
|
Warning Review limit reached
Next review available in: 26 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Summary by QodoAdd strike tracking and quarantine/unquarantine lifecycle for project tasks
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| ) | ||
| await self._record_audit( | ||
| task_id, "task.quarantined", actor, "open", "quarantined", | ||
| project_id=existing["project_id"] if existing else "", |
There was a problem hiding this comment.
WARNING: Hardcoded from_status='open' in quarantine audit — claimed tasks will be logged as transitioning from open instead of claimed.
The quarantine_task method hardcodes from_status="open" in _record_audit, but a task can be quarantined while in claimed state (the WHERE clause only excludes closed, cancelled, quarantined). This causes the audit log to show an incorrect status transition.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| now = time.time() | ||
| cursor = await self._db.execute( | ||
| """UPDATE project_tasks | ||
| SET status = 'open', updated_at = ? |
There was a problem hiding this comment.
WARNING: unquarantine_task returns the task to open without clearing claimed_by/claimed_at. If the task was claimed while quarantined, it becomes stuck: ready_tasks requires claimed_by IS NULL, and claim_task requires claimed_by IS NULL, so the card is unclaimable.
| SET status = 'open', updated_at = ? | |
| SET status = 'open', claimed_by = NULL, claimed_at = NULL, updated_at = ? |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 2 Issues Found | Recommendation: Address before merge Overview| Severity | Count | Issue Details (click to expand)WARNING
Files Reviewed (2 files)
Fix these issues in Kilo Cloud Reviewed by step-3.7-flash · Input: 45.3K · Output: 12.5K · Cached: 169.9K |
|
nemotron-ultra-orB review VERDICT: Clean implementation with good atomicity and error handling. Minor audit log inaccuracy and unused constant.
Automated first-pass review by the nemotron-ultra-orB lane. The lead still reviews before merge. |
Code Review by Qodo
1. Quarantine strands claimed tasks
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| broker: "ProjectEventBroker | None" = None, | ||
| audit: "BoardAuditLog | None" = None, | ||
| project_store: "ProjectStore | None" = None, | ||
| strikes: "StrikeStore | None" = None, | ||
| ) -> None: | ||
| super().__init__(db_path) | ||
| self._broker = broker | ||
| self._audit = audit | ||
| self._project_store = project_store | ||
| self._strikes = strikes |
There was a problem hiding this comment.
2. strikestore not wired lifespan 📜 Skill insight ⌂ Architecture
A new StrikeStore was added and ProjectTaskStore now accepts a strikes dependency, but the app lifespan does not instantiate/init the store, attach it to app.state, or pass it into ProjectTaskStore. This leaves the new strike/quarantine behavior effectively unwired at runtime.
Agent Prompt
## Issue description
`StrikeStore` is introduced but not created/initialized in the application lifespan, nor attached to `app.state`, and it is not passed into `ProjectTaskStore`.
## Issue Context
Compliance requires new stores to be instantiated and attached to `app.state.<name>` during lifespan wiring so routes/services can reliably access them.
## Fix Focus Areas
- tinyagentos/app.py[396-418]
- tinyagentos/app.py[563-582]
- tinyagentos/projects/task_store.py[107-120]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| cursor = await self._db.execute( | ||
| """UPDATE project_tasks | ||
| SET status = 'quarantined', updated_at = ? | ||
| WHERE id = ? AND status NOT IN ('closed', 'cancelled', 'quarantined')""", | ||
| (now, task_id), | ||
| ) | ||
| await self._db.commit() | ||
| changed = cursor.rowcount == 1 | ||
| if changed: | ||
| existing = await self.get_task(task_id) | ||
| if existing is not None: | ||
| await self._publish( | ||
| existing["project_id"], | ||
| "task.quarantined", | ||
| {"id": task_id, "actor": actor}, | ||
| ) | ||
| await self._record_audit( | ||
| task_id, "task.quarantined", actor, "open", "quarantined", | ||
| project_id=existing["project_id"] if existing else "", | ||
| ) | ||
| return changed | ||
|
|
||
| async def unquarantine_task(self, task_id: str, actor: str) -> bool: | ||
| """Return a quarantined task to the open pool and clear its strikes. | ||
|
|
||
| This is the explicit un-quarantine / retry action: the card re-enters | ||
| the ready pool so the fleet may pick it up again. Strikes are cleared | ||
| so a fresh failure count starts from zero. Only acts on a quarantined | ||
| task; returns False otherwise. | ||
| """ | ||
| now = time.time() | ||
| cursor = await self._db.execute( | ||
| """UPDATE project_tasks | ||
| SET status = 'open', updated_at = ? | ||
| WHERE id = ? AND status = 'quarantined'""", | ||
| (now, task_id), | ||
| ) |
There was a problem hiding this comment.
3. Quarantine strands claimed tasks 🐞 Bug ≡ Correctness
ProjectTaskStore.quarantine_task() can quarantine a task in status='claimed' without clearing claimed_by/claimed_at, and unquarantine_task() reopens it without clearing those fields. This can leave a task as status='open' with claimed_by still set, so it never appears in ready_tasks and cannot be claimed or released via existing lifecycle methods.
Agent Prompt
## Issue description
`quarantine_task()` / `unquarantine_task()` update only `status` (and `updated_at`). If a task is quarantined while `status='claimed'`, its `claimed_by`/`claimed_at` remain set; after unquarantine it becomes `status='open'` but still has `claimed_by != NULL`, which prevents it from ever being ready/claimable and also prevents `release_task()` from clearing the claim.
## Issue Context
- `claim_task()` requires `status='open'` and `claimed_by IS NULL`.
- `release_task()` requires `status='claimed'`.
- `ready_tasks` view requires `status='open'` and `claimed_by IS NULL`.
## Fix Focus Areas
- tinyagentos/projects/task_store.py[422-488]
## Suggested fix
- In `quarantine_task()`, either:
- restrict the UPDATE to only quarantine `status='open'`, **or**
- explicitly clear `claimed_by = NULL, claimed_at = NULL` (recommended so quarantining forcibly releases the task).
- In `unquarantine_task()`, ensure the reopened row is in a clean open state by also clearing `claimed_by/claimed_at` (and any other fields that should not persist across retries).
- Add a regression test: claimed -> quarantined -> unquarantined results in `status='open'` and `claimed_by is NULL`, and task appears in ready pool.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| async def record_strike( | ||
| self, | ||
| task_id: str, | ||
| step: str, | ||
| log_tail: str = "", | ||
| actor: str = "", | ||
| ) -> int: | ||
| """Record one verification strike for *task_id*. | ||
|
|
||
| Returns the total strike count for the task after this insert (so the | ||
| caller can decide whether the threshold was just crossed). | ||
| """ | ||
| sid = new_id("str") | ||
| now = time.time() | ||
| await self._db.execute( | ||
| """INSERT INTO task_strikes | ||
| (id, task_id, step, log_tail, actor, created_at) | ||
| VALUES (?, ?, ?, ?, ?, ?)""", | ||
| (sid, task_id, step, log_tail, actor, now), | ||
| ) | ||
| await self._db.commit() | ||
| return await self.count_strikes(task_id) | ||
|
|
There was a problem hiding this comment.
4. Strike total not insert-local 🐞 Bug ☼ Reliability
StrikeStore.record_strike() commits an INSERT and then runs a separate COUNT(*), so the returned total may include other concurrent inserts that occurred between this call’s insert and its count query. Callers therefore cannot reliably infer whether their strike was the one that crossed STRIKE_THRESHOLD from the returned count.
Agent Prompt
## Issue description
`record_strike()` does `INSERT` -> `COMMIT` -> `COUNT(*)`. With concurrent strike recordings for the same task, the count observed by a given caller is not guaranteed to reflect the count immediately after *that caller’s* insert, so threshold-crossing detection based on the returned value can be wrong.
## Issue Context
The `record_strike()` docstring states the returned total is used by the caller to decide whether the threshold was just crossed.
## Fix Focus Areas
- tinyagentos/projects/strike_store.py[41-63]
## Suggested fix
- Serialize insert+count for a task by using a transaction that holds a write lock while determining the post-insert total, e.g.:
- `BEGIN IMMEDIATE; SELECT COUNT(*) ...; INSERT ...; SELECT COUNT(*) ...; COMMIT;`
- (or return `(previous_count, new_count)` computed inside the same transaction).
- If quarantine triggering is the goal, consider moving “threshold crossed” handling into the same critical section (or make the action idempotent, e.g., quarantine is safe to call multiple times).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| cursor = await self._db.execute( | ||
| """UPDATE project_tasks | ||
| SET status = 'quarantined', updated_at = ? | ||
| WHERE id = ? AND status NOT IN ('closed', 'cancelled', 'quarantined')""", | ||
| (now, task_id), | ||
| ) | ||
| await self._db.commit() | ||
| changed = cursor.rowcount == 1 | ||
| if changed: | ||
| existing = await self.get_task(task_id) | ||
| if existing is not None: | ||
| await self._publish( | ||
| existing["project_id"], | ||
| "task.quarantined", | ||
| {"id": task_id, "actor": actor}, | ||
| ) | ||
| await self._record_audit( | ||
| task_id, "task.quarantined", actor, "open", "quarantined", | ||
| project_id=existing["project_id"] if existing else "", | ||
| ) |
There was a problem hiding this comment.
5. Quarantine audit status wrong 🐞 Bug ≡ Correctness
ProjectTaskStore.quarantine_task() records an audit transition from_status='open' even though its UPDATE can quarantine tasks from other statuses (e.g., 'claimed'). This produces incorrect audit history for quarantines.
Agent Prompt
## Issue description
`quarantine_task()` hard-codes the audit transition as `open -> quarantined`, but the SQL guard allows quarantining any status except `closed/cancelled/quarantined` (so `claimed` etc. are possible). This corrupts the audit trail.
## Issue Context
Audit history is used as the append-only board activity log, so incorrect `from_status` reduces trust in operational forensics.
## Fix Focus Areas
- tinyagentos/projects/task_store.py[422-451]
## Suggested fix
- Capture the actual prior status in a race-safe way:
- Use `BEGIN IMMEDIATE` and `SELECT status, claimed_by FROM project_tasks WHERE id=?` before the UPDATE, then pass that status into `_record_audit`.
- Alternatively restrict quarantine to `status='open'` only, if that is the intended lifecycle.
- Add a test that quarantining a claimed task records `from_status='claimed'`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| await self._publish( | ||
| existing["project_id"], | ||
| "task.quarantined", | ||
| {"id": task_id, "actor": actor}, | ||
| ) |
There was a problem hiding this comment.
6. Quarantine events dropped by bridge 🐞 Bug ◔ Observability
ProjectTaskStore publishes 'task.quarantined' and 'task.unquarantined' events, but
BeadsBridge.on_event() returns early unless kind is in
('task.claimed','task.released','task.closed'). Therefore quarantine/unquarantine events will not be
forwarded to the a2a chat channel by the bridge.
Agent Prompt
## Issue description
`ProjectTaskStore` emits `task.quarantined` / `task.unquarantined`, but the Beads bridge filters out all event kinds except claimed/released/closed, so these new events never produce a2a system announcements.
## Issue Context
This reduces visibility into quarantine state changes compared to other lifecycle actions that are announced.
## Fix Focus Areas
- tinyagentos/projects/task_store.py[422-488]
- tinyagentos/projects/beads_bridge.py[278-321]
## Suggested fix
- Extend the allowlist in `BeadsBridge.on_event()` to include `task.quarantined` and `task.unquarantined`.
- Add formatting/handling for the new events (e.g., message includes actor + task id + title, similar to claimed/closed).
- Add an integration test ensuring these events generate a bridge message.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
nemotron-ultra-kilo review Code Review
|
| Severity | Count |
|---|---|
| Correctness bugs | 6 |
| Security issues | 2 |
| Missing tests | 8 |
| Style issues | 5 |
Priority fixes:
- Fix missing
commit()instrike_store.py - Fix undefined variables in
task_store.pyrecord_strikefragment (diff artifact) - Add transaction wrapping for
quarantine_task/unquarantine_task/close_taskstrike clearing - Add authorization to quarantine actions
- Add composite index
(task_id, created_at)to strike schema - Write integration tests for strike threshold → quarantine flow
Code Review
tinyagentos/projects/strike_store.py (new file)
Correctness Bugs
1. Missing commit() after INSERT — strike_store.py:67-71
await self.execute("INSERT ...", (strike_id, task_id, step, log_tail, actor, time.time()))
row = await self.fetchone("SELECT COUNT(*) ...", (task_id,))
return row[0] if row else 0No await self.commit() after the INSERT. With SQLite in default autocommit mode (unless isolation_level=None), this may still work, but it's non-deterministic and breaks if isolation level is set. The diff version in task_store.py explicitly commits — this file doesn't. Fix: add await self.commit() after the INSERT.
2. Race condition in record_strike count — strike_store.py:67-73
Two concurrent calls to record_strike(task_id, ...) can both read the same count before either commits, returning the same value. The diff version in task_store.py uses count_strikes() after commit which has the same race. Consider using RETURNING COUNT(*) (SQLite 3.35+) or INSERT ... RETURNING to get atomic count, or wrap in a transaction with BEGIN IMMEDIATE.
3. new_id() called without "str" argument — strike_store.py:63
strike_id = new_id() # diff shows new_id("str")The diff shows new_id("str") but the actual file uses new_id(). Check tinyagentos/projects/ids.py signature — if it requires an argument, this is a bug.
4. Schema mismatch: column name — strike_store.py:11 vs task_store.py:379
Schema defines column step (line 11), but task_store.py:379 inserts into step column — this matches. However the INSERT in task_store.py:378-384 uses columns (id, task_id, step, log_tail, actor, created_at) while the diff version of strike_store.py uses (id, task_id, step, log_tail, actor, created_at) — these match. Wait, looking at the diff version of strike_store.py line 33-40:
INSERT INTO task_strikes
(id, task_id, step, log_tail, actor, created_at)
VALUES (?, ?, ?, ?, ?, ?)This matches. OK.
5. Inconsistent column name: task_id vs task_id — OK, matches.
6. Missing index on created_at for ordering — strike_store.py:13
The idx_strikes_task index only covers task_id. Queries ordering by created_at (e.g., list_strikes, latest) will need a composite index (task_id, created_at) for efficiency.
Security Issues
7. SQL injection risk — none found. All queries use parameterized ? placeholders correctly.
8. No input validation on task_id, step, log_tail, actor — Could inject very long strings causing DoS via DB bloat. Consider length limits on log_tail and actor.
Missing/Weak Test Cases
9. No tests for race condition in record_strike — Two concurrent strikes on same task_id should both increment count correctly.
10. No test for clear_strikes on non-existent task_id — Should return 0, not error.
11. No test for STRIKE_THRESHOLD integration — The constant is defined but not used in this file; integration test needed with quarantine_task.
12. No test for list_strikes ordering — Verify ORDER BY created_at ASC.
13. No test for latest() returning None on empty — Edge case.
Style / Convention Issues
14. Inconsistent method naming — record_strike vs diff's record_strike (same), but get_strikes vs diff's list_strikes, clear_strikes vs diff's clear_strikes. Pick one convention.
15. Missing type hints on _row function — def _row(r) -> dict:
16. STRIKE_THRESHOLD unused in this file — Defined but never referenced. Either use it in record_strike (return bool whether threshold crossed) or remove.
17. _row helper defined at module level but could be a staticmethod — Minor.
18. Docstring says STRIKE_THRESHOLD = 3 but no enforcement — The threshold logic lives in task_store.py (dispatch host). Document that clearly.
tinyagentos/projects/task_store.py (modified)
Correctness Bugs
19. record_strike call uses undefined variables — task_store.py:378-384
sid = new_id("str")
now = time.time()
await self._db.execute(
"""INSERT INTO task_strikes
(id, task_id, step, log_tail, actor, created_at)
VALUES (?, ?, ?, ?, ?, ?)""",
(sid, task_id, step, log_tail, actor, now),
)Variables task_id, step, log_tail, actor are not defined in this scope. This is a diff artifact — the actual method signature and call site are missing from the diff. This will not run. The diff shows an INSERT but no surrounding method. Looking at context, this appears to be a fragment from a record_strike method that was supposed to be added to TaskStore but the method definition is missing from the diff.
20. Missing commit() after INSERT in diff fragment — task_store.py:384
await self._db.execute(...)
await self._db.commit() # <-- MISSING in diff
return await self.count_strikes(task_id)The diff shows await self._db.commit() on line 385? Wait, line 385 in diff shows await self._db.commit() — yes it's there at line 385. OK.
21. quarantine_task and unquarantine_task don't use transactions — Multiple DB operations (UPDATE, then SELECT, then publish, then audit) without atomic transaction. If publish fails, the DB is already committed. Use async with self._db.transaction(): or wrap in try/except with rollback.
22. Race condition in quarantine_task — task_store.py:379-386
cursor = await self._db.execute(
"""UPDATE project_tasks
SET status = 'quarantined', updated_at = ?
WHERE id = ? AND status NOT IN ('closed', 'cancelled', 'quarantined')""",
(now, task_id),
)
await self._db.commit()
changed = cursor.rowcount == 1
if changed:
existing = await self.get_task(task_id) # Race: task could change hereBetween UPDATE and get_task, another process could modify the task. Fetch existing before UPDATE using SELECT ... FOR UPDATE (SQLite: SELECT ... FROM project_tasks WHERE id = ? AND status NOT IN (...) FOR UPDATE) or do it in a single transaction.
23. unquarantine_task clears strikes after commit — task_store.py:447-450
await self._db.commit()
changed = cursor.rowcount == 1
if changed:
if self._strikes is not None:
try:
await self._strikes.clear_strikes(task_id)If clear_strikes fails, strikes remain but task is unquarantined. Should clear strikes before commit, or wrap in same transaction.
24. close_task clears strikes with bare except Exception — task_store.py:385-391
try:
await self._strikes.clear_strikes(task_id)
except Exception:
logger.warning("clear_strikes failed for task %s on close", task_id, exc_info=True)Swallows all exceptions silently (only logs). If strikes table is locked or DB error, strikes persist on closed task. At minimum, re-raise or have a retry.
25. close_task commits before clearing strikes — task_store.py:381-391
# close logic...
await self._db.commit()
# then clear strikes
if self._strikes is not None:
try:
await self._strikes.clear_strikes(task_id)The close commits, then strikes cleared in separate transaction. If clear fails, task is closed but strikes remain. Should be atomic.
Security Issues
26. No authorization check in quarantine_task / unquarantine_task — Any actor can quarantine/unquarantine any task. Need permission check (is actor a lead?).
27. actor parameter passed directly to audit log — No sanitization. If actor comes from untrusted input, could inject log entries. Low risk but worth noting.
Missing/Weak Test Cases
28. No test for quarantine_task on already-quarantined task — Should return False.
29. No test for quarantine_task on closed/cancelled task — Should return False.
30. No test for unquarantine_task on non-quarantined task — Should return False.
31. No test that strikes are cleared on unquarantine_task — Critical integration test.
32. No test that strikes are cleared on close_task — Critical integration test.
33. No test for concurrent quarantine_task + unquarantine_task — Race condition.
34. No test for quarantine_task publishing task.quarantined event — Event emission test.
35. No test for unquarantine_task publishing task.unquarantined event — Event emission test.
Style / Convention Issues
36. Inconsistent parameter naming — actor vs closed_by vs reopened_by. Use consistent naming (actor everywhere or by).
37. Magic string statuses — 'quarantined', 'closed', 'cancelled', 'open' scattered as string literals. Define as constants/enum.
38. quarantine_task docstring says "fleet will not pick it up" but no enforcement shown — Document where the ready-pool query filters out quarantined.
39. Missing type hints on quarantine_task return — -> bool is there, OK.
40. Long lines — Several lines exceed 88 chars (e.g., line 379, 419).
Summary
| Severity | Count |
|---|---|
| Correctness bugs | 6 |
| Security issues | 2 |
| Missing tests | 8 |
| Style issues | 5 |
Priority fixes:
- Fix missing
commit()instrike_store.py - Fix undefined variables in
task_store.pyrecord_strikefragment (diff artifact) - Add transaction wrapping for
quarantine_task/unquarantine_task/close_taskstrike clearing - Add authorization to quarantine actions
- Add composite index
(task_id, created_at)to strike schema - Write integration tests for strike threshold → quarantine flow
Automated first-pass review by the nemotron-ultra-kilo lane. The lead still reviews before merge.
|
Holding this one, and part of the reason is my card's fault rather than yours. The store design itself is good: append-only strike log, indexed, clear docstrings, auto-clear on close or merge. But it does not yet do the thing the card exists for. BLOCKER, and qodo is right: That is worth naming plainly, because it is the exact failure this card was written to fix: a feature whose entire purpose is to make silent failures loud, shipped in a state where it silently does nothing. CI is green because the code parses, not because it runs. Also missing against the card, and I should say which of these is my fault:
kilo's two findings, one verified and one I could not:
To land: wire the store in the lifespan, add the three tests, add the board state and the lead notification, and fold the doc plus CHANGELOG. The store file itself needs little change. |
|
nemotron-super review VERDICT: No blocking issues found. Automated first-pass review by the nemotron-super lane. The lead still reviews before merge. |
|
Closing this, and the fault is mine rather than the lane's. What is here: So this is a store that never runs. Merging it would put unreferenced code on The root cause is my card, tsk-glxi4e. I wrote it as "make quarantine loud" without specifying the wiring, the tests, the surface or the docs, so a lane built precisely what was asked and the contract was wrong. I own that; the card body is the lead's job and mine was underspecified. Re-carding it as a complete vertical slice: store, lifespan wiring, the actual loud surfacing (a quarantined card must become visible rather than silently parked), tests that prove a 3-struck card is reported, and docs. The work on this branch is not wasted, it is the store layer of that slice, and the new card will point at this branch so it can be reused rather than rewritten. Real-world evidence for why the loud part matters, gathered tonight after this PR was opened: three cards sat 3-struck and invisible until I swept the strike files by hand, one of them since 2026-07-21. One was on another lead's board entirely. That is the failure this card exists to end, and a store nobody constructs does not end it. |
1 participant
Autonomous build of board card tsk-glxi4e.
Files:
tinyagentos/projects/strike_store.py | 98 ++++++++++++++++++++++++++++++++++++
tinyagentos/projects/task_store.py | 79 +++++++++++++++++++++++++++++
2 files changed, 177 insertions(+)
Summary by Gitar
StrikeStoreto record verification failures and threshold limitsquarantine_taskandunquarantine_taskmethods inProjectTaskStoreThis will update automatically on new commits.