Make 3-strike QUARANTINE loud: store + lifespan wiring + surfacing + tests (FULL slice, replaces tsk-glxi4e) - #2333
Make 3-strike QUARANTINE loud: store + lifespan wiring + surfacing + tests (FULL slice, replaces tsk-glxi4e)#2333jaylfc wants to merge 1 commit into
Conversation
|
ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing |
📝 WalkthroughWalkthroughThe change adds an asynchronous SQLite-backed ChangesTask strike and quarantine lifecycle
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant ProjectTaskStore
participant StrikeStore
participant SQLite
Caller->>ProjectTaskStore: quarantine_task(task_id, actor)
ProjectTaskStore->>SQLite: set task state to quarantined
ProjectTaskStore-->>Caller: return result
Caller->>ProjectTaskStore: unquarantine_task(task_id, actor)
ProjectTaskStore->>SQLite: set task state to open
ProjectTaskStore->>StrikeStore: clear_strikes(task_id)
StrikeStore->>SQLite: delete task strikes
StrikeStore-->>ProjectTaskStore: return deleted count
ProjectTaskStore-->>Caller: return result
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tinyagentos/projects/strike_store.py (1)
41-98: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd tests for the strike lifecycle.
The PR objective requires tests, but no test file changed.
tinyagentos/projects/strike_store.py#L41-L98: add async SQLite tests for recording, counting, ordering, latest lookup, and deletion counts.tinyagentos/projects/task_store.py#L362-L489: add lifecycle tests for close cleanup, quarantine eligibility, claimed-task quarantine, successful unquarantine cleanup, and cleanup failure behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tinyagentos/projects/strike_store.py` around lines 41 - 98, Add async SQLite tests covering the strike lifecycle around StrikeStore.record_strike, count_strikes, list_strikes, latest, and clear_strikes: verify recording/counts, oldest-first ordering, latest lookup, and deletion counts. Also add lifecycle tests in tinyagentos/projects/task_store.py:362-489 covering close cleanup, quarantine eligibility, claimed-task quarantine, successful unquarantine cleanup, and cleanup failure behavior; the strike_store.py:41-98 implementation requires no direct change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tinyagentos/projects/task_store.py`:
- Around line 449-452: Update the quarantine transition audit call to use the
committed pre-transition status instead of hardcoding "open", preserving
"claimed" when applicable. Also update close_task’s audit transition to use the
task’s actual prior status, including "quarantined", rather than always
recording "open".
- Around line 473-477: Update the unquarantine transition around clear_strikes
so failure to remove strikes prevents a successful transition: perform strike
cleanup before committing the task as open and publishing the unquarantine
event, or restore the quarantined state before returning failure. Ensure the
method does not return True or emit an unquarantine event when clear_strikes
fails.
- Around line 433-452: Update the quarantine status transition in the
task-quarantine method to set both claimed_by and claimed_at to NULL, allowing
claimed tasks to return to the ready pool after unquarantine. Preserve existing
behavior for unclaimed tasks, and record claimed as the audit source status when
the task was claimed.
---
Nitpick comments:
In `@tinyagentos/projects/strike_store.py`:
- Around line 41-98: Add async SQLite tests covering the strike lifecycle around
StrikeStore.record_strike, count_strikes, list_strikes, latest, and
clear_strikes: verify recording/counts, oldest-first ordering, latest lookup,
and deletion counts. Also add lifecycle tests in
tinyagentos/projects/task_store.py:362-489 covering close cleanup, quarantine
eligibility, claimed-task quarantine, successful unquarantine cleanup, and
cleanup failure behavior; the strike_store.py:41-98 implementation requires no
direct change.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a9a850a5-ca63-4e05-8707-ae6670d43a83
📒 Files selected for processing (2)
tinyagentos/projects/strike_store.pytinyagentos/projects/task_store.py
| 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.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Clear the active claim when quarantine accepts a claimed task.
Line 436 accepts claimed tasks. The update retains claimed_by and claimed_at. unquarantine_task later changes only status to open. claim_task requires claimed_by IS NULL, so the task cannot return to the ready pool.
Clear both claim fields when quarantining. Also record claimed as the audit source status when applicable.
Proposed state fix
UPDATE project_tasks
-SET status = 'quarantined', updated_at = ?
+SET status = 'quarantined',
+ claimed_by = NULL,
+ claimed_at = NULL,
+ updated_at = ?
WHERE id = ? AND status NOT IN ('closed', 'cancelled', 'quarantined')🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tinyagentos/projects/task_store.py` around lines 433 - 452, Update the
quarantine status transition in the task-quarantine method to set both
claimed_by and claimed_at to NULL, allowing claimed tasks to return to the ready
pool after unquarantine. Preserve existing behavior for unclaimed tasks, and
record claimed as the audit source status when the task was claimed.
| 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.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Preserve the actual source status in audit records.
This audit entry always records open -> quarantined, although the update also permits claimed -> quarantined. The new quarantined status also makes close_task record open -> closed for a quarantined task. Preserve the committed pre-transition status in each audit record.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tinyagentos/projects/task_store.py` around lines 449 - 452, Update the
quarantine transition audit call to use the committed pre-transition status
instead of hardcoding "open", preserving "claimed" when applicable. Also update
close_task’s audit transition to use the task’s actual prior status, including
"quarantined", rather than always recording "open".
| if self._strikes is not None: | ||
| try: | ||
| await self._strikes.clear_strikes(task_id) | ||
| except Exception: | ||
| logger.warning("clear_strikes failed for task %s on unquarantine", task_id, exc_info=True) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not complete unquarantine when strike cleanup fails.
The task is committed as open before cleanup. If clear_strikes fails, this method still returns True and publishes an unquarantine event with the old strike count intact. The next failure can immediately meet the quarantine threshold, which contradicts the documented fresh count.
Make strike deletion a precondition for the successful transition, or compensate by restoring quarantine before returning failure.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tinyagentos/projects/task_store.py` around lines 473 - 477, Update the
unquarantine transition around clear_strikes so failure to remove strikes
prevents a successful transition: perform strike cleanup before committing the
task as open and publishing the unquarantine event, or restore the quarantined
state before returning failure. Ensure the method does not return True or emit
an unquarantine event when clear_strikes fails.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| {"id": task_id, "actor": actor}, | ||
| ) | ||
| await self._record_audit( | ||
| task_id, "task.quarantined", actor, "open", "quarantined", |
There was a problem hiding this comment.
WARNING: quarantine_task hardcodes from_status="open" in _record_audit, but the WHERE clause allows quarantining tasks in claimed (and any non-excluded) status. A claimed task quarantined here will record open -> quarantined in the audit log instead of the actual prior status, corrupting the audit trail.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
|
|
||
| def _row(r) -> dict: | ||
| keys = ("id", "task_id", "step", "log_tail", "actor", "created_at") | ||
| return dict(zip(keys, r)) |
There was a problem hiding this comment.
SUGGESTION: _row() hardcodes the column-key mapping. If STRIKE_SCHEMA ever changes (column add/remove/reorder), this silently produces incorrect dictionaries rather than failing loudly. Consider deriving keys from the schema or using cur.description like _row_to_task does elsewhere in the codebase.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (2 files)
Fix these issues in Kilo Cloud Reviewed by step-3.7-flash · Input: 113.4K · Output: 14.7K · Cached: 297.3K |
|
nemotron-super review VERDICT: No blocking issues found. Automated first-pass review by the nemotron-super lane. The lead still reviews before merge. |
|
BLOCKING - not wired in, the exact inert-param shape. The store and the two task_store methods are sound in isolation, but nothing in this diff (or on dev) constructs StrikeStore or passes strikes= to ProjectTaskStore, so every new path is a production no-op behind a None default; quarantine_task/unquarantine_task have zero callers (no route, nothing sets the status); record_strike is never called; and the card explicitly demands the FULL slice - store + lifespan wiring + surfacing + tests - while the diff has no lifespan change, no route, no UI surfacing, and no test file (the automated scope warning in the body already flags the tests gap). tsk-orqoif exists precisely because tsk-glxi4e was superseded for being a partial slice; merging this would repeat that. Needs: lifespan construction + injection, the unquarantine route the docstrings reference, board surfacing of the quarantined status, and tests that fail if the wiring is dropped. Fix-forward on this branch. |
jaylfc
added
the
lead-blocked
1 participant
CARD TITLE (intent, not commit subject): Make 3-strike QUARANTINE loud: store + lifespan wiring + surfacing + tests (FULL slice, replaces tsk-glxi4e)
Autonomous build of board card tsk-orqoif.
Files:
tinyagentos/projects/strike_store.py | 98 ++++++++++++++++++++++++++++++++++++
tinyagentos/projects/task_store.py | 79 +++++++++++++++++++++++++++++
2 files changed, 177 insertions(+)
Summary by Gitar
StrikeStorefor tracking verification strikes withSTRIKE_THRESHOLDset to 3quarantine_taskandunquarantine_taskmethods toProjectTaskStorewith automatic strike clearingThis will update automatically on new commits.
Summary by CodeRabbit