Quarantine audit trail records a false from_status; claimable route undocumented - #2345
Conversation
…le route
quarantine_task hardcoded from_status="open" in _record_audit, but its
WHERE clause permits quarantining a claimed card, so the audit trail logged
a false "open to quarantined" transition for claimed cards. Mirror
close_task's race-free derivation from the committed row's claimed_by so
the audit records the true pre-quarantine status. Add a test that
quarantines a claimed task and asserts from_status == "claimed".
Also document the LEAD-only mark-task-claimable route (POST
.../tasks/{id}/claimable) in the project_tasks scope bullet of
docs/agent-coordination.md -- it was allowlisted in auth_middleware.py
but never documented, unlike the unquarantine route added with the
strike-store wiring.
|
ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe quarantine audit now records ChangesQuarantine audit status
Claimable endpoint documentation
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 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 |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| # 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.
WARNING: from_status derivation is incomplete for non-claimed statuses
The ternary collapses every status except "claimed" to "open", but quarantine_task's UPDATE guard also allows status = 'reopened' tasks (only closed/cancelled/quarantined are excluded). If a reopened task is quarantined, the audit record will incorrectly show "open" → "quarantined" instead of "reopened" → "quarantined". The same two-value pattern exists in close_task (line 393), so this is at least partially pre-existing, but the new code replicates it without addressing it. Consider deriving from existing["status"] directly, or documenting the intentional simplification.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_quarantine_claimed_task_records_actual_from_status(tmp_path): |
There was a problem hiding this comment.
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 from_status ternary were accidentally inverted (e.g. "open" if existing.get("claimed_by") else "claimed"), the open-path audit record would silently break without any test catching it. Add a companion assertion that quarantining an unclaimed task records from_status == "open".
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| only. Granting project_tasks also makes the agent a project member. | ||
| `POST .../tasks/{id}/claimable` is also reachable, but LEAD-only: the | ||
| route (`_authorize_project_lead`) refuses a plain project_tasks worker. | ||
| It toggles only the `claimable` label (the fleet-pickup flag), preserving |
There was a problem hiding this comment.
SUGGESTION: claimable route description is ambiguous about label semantics
"It toggles only the claimable label" could be read as replacing all labels with ["claimable"] rather than adding/removing it in place. The unquarantine entry below it avoids this ambiguity by describing its behaviour explicitly ("clears its strikes"). Consider rephrasing to: "It adds or removes the claimable label while preserving every other label" to match the precision of the neighbouring bullet.
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 (3 files)
Fix these issues in Kilo Cloud Reviewed by step-3.7-flash · Input: 48.3K · Output: 4.4K · Cached: 161.8K |
|
nemotron-super review VERDICT: Approved Automated first-pass review by the nemotron-super lane. The lead still reviews before merge. |
|
Correction to my kilo adjudication above: I wrote that |
1 participant
CARD TITLE (intent, not commit subject): Quarantine audit trail records a false from_status; claimable route undocumented
Autonomous build of board card tsk-dtanmd.
quarantine_task hardcoded from_status="open" in _record_audit, but its
WHERE clause permits quarantining a claimed card, so the audit trail logged
a false "open to quarantined" transition for claimed cards. Mirror
close_task's race-free derivation from the committed row's claimed_by so
the audit records the true pre-quarantine status. Add a test that
quarantines a claimed task and asserts from_status == "claimed".
Also document the LEAD-only mark-task-claimable route (POST
.../tasks/{id}/claimable) in the project_tasks scope bullet of
docs/agent-coordination.md -- it was allowlisted in auth_middleware.py
but never documented, unlike the unquarantine route added with the
strike-store wiring.
Files:
docs/agent-coordination.md | 5 +++++
tests/test_task_store.py | 21 +++++++++++++++++++++
tinyagentos/projects/task_store.py | 7 ++++++-
3 files changed, 32 insertions(+), 1 deletion(-)
Summary by CodeRabbit
Bug Fixes
Documentation