Checklist carry take 2: fix event scope + None-safety from #2473, resolve conflict (supersedes tsk-gzwv3x) - #2480
Checklist carry take 2: fix event scope + None-safety from #2473, resolve conflict (supersedes tsk-gzwv3x)#2480jaylfc wants to merge 2 commits into
Conversation
Carry the OS-owned objective checklist forward from exec/tsk-w2do7j onto current
origin/dev as a single squash commit. Adds the checklist model, the cki id
prefix, the POST/GET /api/projects/{project_id}/tasks/{task_id}/checklist-items
routes, and the route + store tests.
Only docs/agent-coordination.md conflicted; tinyagentos/routes/projects.py and
the store files merged clean.
Conflict resolutions (docs/agent-coordination.md):
- dev's copy gained the "## Agent-token API surface (Bearer allowlist)" section
(from #2430) and inserted the "Agent memory mode" and "Cluster node revoke"
sections in the slot the branch used for its "## Task checklist items"
section. Resolved by keeping dev's sections and restoring the #2415 checklist
section (list/create shapes, 404 existence-hiding, activity-feed logging,
archive rules) immediately before "## Answering a select decision".
- The #2415 checklist section stated the routes were NOT agent-reachable
(refused 401 at the allowlist, pinned by two strict xfails). dev's allowlist
already matches the checklist paths (per #2430), so carrying #2415 forward
makes the routes agent-Bearer-reachable. The two strict xfails
(test_project_tasks_create_may_author, test_project_tasks_may_read) are
promoted to positive assertions (200), and
test_project_tasks_alone_may_NOT_author now pins the scope split as a 403 --
a project_tasks read token is refused POST because it lacks the
project_tasks_create grant, which is the behaviour that test's own docstring
described as its goal once the allowlist gap closed.
- dev's Bearer-allowlist subsection credited the LIST route to the
project_tasks_create scope; the handler uses the default project_tasks read
scope for GET and project_tasks_create for POST. Corrected so the doc matches
the code and the restored checklist section.
Behaviour change vs the original branch (semantic drift, called out as required
for a carry-forward): on #2415 the checklist routes were unreachable by agent
tokens (401 at the allowlist); after the carry-forward they are
agent-Bearer-reachable and handler-scope-gated, matching dev's already-widened
allowlist and #2415's own route docstrings. No store, route, or ids code path
from #2415 was weakened or altered; only the stale "unreachable" docs/tests
were reconciled to the live allowlist.
Supersedes: #2415
Docs-Reviewed: docs/agent-coordination.md was edited to add the task checklist
routes section (reconciled with the Bearer-allowlist section), correct the LIST
route scope from project_tasks_create to project_tasks, and restore the
checklist section in dev's section order; changelog fragment renamed to the
tsk-gzwv3x naming convention.
…lose_task guard Replaces #2473 Three verified defects in the OS-owned checklist carry (#2415, PR #2473): 1. Event scope: create_checklist_item and archive_checklist_item published checklist.item.created/archived under task_id, but project subscribers subscribe at project_id scope. Now resolves the task's project_id (mirroring sibling task mutations) and publishes under it. 2. None-safety: archive_checklist_item indexed item[verified] where get_checklist_item returns dict | None, raising TypeError on a missing item. Now raises ValueError(checklist item not found: ...). 3. Return annotation: update_checklist_item declared return dict but returns get_checklist_item(...) which is dict | None. Corrected to dict | None. Kilo SUGGESTION #2 (~1282 redundant get_task): adopted. The create_checklist_item route called store.get_task(task_id) after _require_task_in_project already fetched and validated the task. Now reuses the task dict from the guard. Kilo SUGGESTION #1 (~1254 mixin pattern for CreateChecklistItemIn): declined. The auth+guard boilerplate is already factored into _authorize_task_actor and _require_task_in_project across all task routes; a shared mixin would scope-creep beyond this PR and is better as a separate refactor. Merged dev close_task ownership guard (force parameter, #2287): close_task and the close route now refuse a claimer mismatch unless the caller is the project lead, owner, or a session admin. Red proof (BASE branch, pre-fix): uv run --group dev pytest tests/projects/test_task_store.py::test_checklist_item_event_delivered_at_project_scope tests/projects/test_task_store.py::test_archive_nonexistent_item_raises_value_error -q 2 failed (TypeError on None; no event at project scope) Green (post-fix): uv run --group dev pytest tests/projects/test_task_store.py tests/test_routes_task_checklist.py tests/test_routes_projects_agent_tasks.py tests/test_task_store.py tests/test_routes_tasks.py tests/test_project_task_store.py tests/test_task_lifecycle_notifications.py tests/test_project_events.py tests/projects/test_event_broker_integration.py tests/projects/test_event_broker.py -q 227 passed Docs-Reviewed: Checklist route docs in docs/agent-coordination.md (lines 837-922) already match the route surface and remain accurate. The close_task ownership-guard behavior is merged from dev and covered by changelog.d/2287-close-ownership-guard.md. The redundant-get_task cleanup changes no API surface.
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
|
Warning Review limit reached
Next review available in: 5 minutes Limit details: You’ve used all 2 included reviews currently available under your plan. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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 (10)
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}
| ) | ||
| ok = await store.close_task(task_id, closed_by=closed_by, reason=payload.reason, force=force) | ||
| if not ok: | ||
| if existing.get("claimed_by") and existing["claimed_by"] != closed_by: |
There was a problem hiding this comment.
WARNING: Post-close 409 check uses stale existing dict
The existing task snapshot is fetched at line 1006, before store.close_task(...) runs. The store's force=True path does NOT clear claimed_by in the database, so after a successful force-close the stale existing["claimed_by"] still holds the original claimer id. The comparison existing["claimed_by"] != closed_by then incorrectly returns 409 for every force-close where closed_by differs from the claimer.
This breaks the ownership-guard bypass for admin/owner session callers. For example, test_admin_session_closes_agent_claimed_card_lead_is_agent sends {"closed_by": ctx.uid} (the admin's user id), which is never equal to the agent claimer's canonical id — the route returns 409 even though the store already closed the task.
Fix: re-fetch the task after store.close_task and use the live claimed_by value for the check, e.g.:
current = await store.get_task(task_id) if not ok else None
if not ok:
if current and current.get("claimed_by") and current["claimed_by"] != actor_id:
return JSONResponse({"error": "not claimed by you"}, status_code=409)
return JSONResponse({"error": "cannot close"}, status_code=409)
Also, compare against actor_id (the authenticated identity) rather than closed_by, since _resolve_actor can return a body-supplied value that differs from the token for session callers.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| item = _row_to_checklist_item(row, desc) | ||
| task = await self.get_task(task_id) | ||
| project_id = task["project_id"] if task is not None else "" | ||
| await self._publish(project_id, "checklist.item.created", {"id": item["id"], "text": item["text"], "task_id": task_id}) |
There was a problem hiding this comment.
SUGGESTION: Redundant get_task call in create_checklist_item
Line 760 calls self.get_task(task_id) solely to resolve project_id for the _publish call. The route handler at projects.py:1290 already called _require_task_in_project, which performed the same get_task lookup and validated the task exists in this project. The PR description ("Kilo SUGGESTION #2 adopted") says this redundant call was removed, but the store-layer fetch remains.
Concretely: if the task were ever deleted between the route guard and this store call, task would be None, project_id would collapse to "", and the event would be published to an empty scope — silently dropped rather than raising. Reusing the task dict already held by the caller (passing project_id as a parameter) would eliminate both the redundant round-trip and this silent-drop edge case.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (6 files)
Fix these issues in Kilo Cloud Reviewed by step-3.7-flash · Input: 112.8K · Output: 21.9K · Cached: 2.2M |
|
nemotron-super review VERDICT: Pass Automated first-pass review by the nemotron-super lane. The lead still reviews before merge. |
|
nemotron-super review VERDICT: Pass Automated first-pass review by the nemotron-super lane. The lead still reviews before merge. |
|
nemotron-super review VERDICT: OK
Automated first-pass review by the nemotron-super lane. The lead still reviews before merge. |
|
HELD + CLOSED as superseded -> card tsk-y44sls (checklist carry take 3). Adjudication: the work is still wanted (the checklist feature never landed on dev - no create/update/archive_checklist_item exists on current dev), but this branch is a revision-on-top-of-revision (exec/tsk-uby6uh cut on exec/tsk-gzwv3x), 8 days stale, 143 commits behind dev, and CONFLICTING in 4 files (routes/projects.py, tests/projects/test_task_store.py, tests/test_routes_projects_agent_tasks.py, docs/agent-coordination.md - trial-merged against 40a9651 to measure). Per fleet supersede flow the replacement is a fresh branch from current dev; tsk-y44sls carries the full spec: the feature, the 3 verified defect fixes from this PR's adjudication (event scope to project_id, archive None-safety ValueError, dict|None annotation), the adopted redundant-get_task cleanup, red-proof requirements, and a note NOT to re-carry the close_task ownership guard (already on dev via #2287). |
1 participant
CARD TITLE (intent, not commit subject): Checklist carry take 2: fix event scope + None-safety from #2473, resolve conflict (supersedes tsk-gzwv3x)
Autonomous build of board card tsk-uby6uh.
REVISION: built on
exec/tsk-gzwv3x(cut atd3156e85bcba5215023e2ea5a88ff74b2cf738d6), not ondev. That branch'scommits are ancestors of this one and the
Files:list below is the diff SINCE it,so this PR shows the revision alone while carrying the original work. Verified by
git merge-base --is-ancestorbefore the PR was opened.Replaces #2473
Three verified defects in the OS-owned checklist carry (#2415, PR #2473):
Event scope: create_checklist_item and archive_checklist_item published
checklist.item.created/archived under task_id, but project subscribers
subscribe at project_id scope. Now resolves the task's project_id (mirroring
sibling task mutations) and publishes under it.
None-safety: archive_checklist_item indexed item[verified] where
get_checklist_item returns dict | None, raising TypeError on a missing
item. Now raises ValueError(checklist item not found: ...).
Return annotation: update_checklist_item declared return dict but returns
get_checklist_item(...) which is dict | None. Corrected to dict | None.
Kilo SUGGESTION #2 (~1282 redundant get_task): adopted. The create_checklist_item
route called store.get_task(task_id) after _require_task_in_project already
fetched and validated the task. Now reuses the task dict from the guard.
Kilo SUGGESTION #1 (~1254 mixin pattern for CreateChecklistItemIn): declined.
The auth+guard boilerplate is already factored into _authorize_task_actor and
_require_task_in_project across all task routes; a shared mixin would scope-creep
beyond this PR and is better as a separate refactor.
Merged dev close_task ownership guard (force parameter, #2287): close_task and
the close route now refuse a claimer mismatch unless the caller is the project
lead, owner, or a session admin.
Red proof (BASE branch, pre-fix):
uv run --group dev pytest tests/projects/test_task_store.py::test_checklist_item_event_delivered_at_project_scope tests/projects/test_task_store.py::test_archive_nonexistent_item_raises_value_error -q
2 failed (TypeError on None; no event at project scope)
Green (post-fix):
uv run --group dev pytest tests/projects/test_task_store.py tests/test_routes_task_checklist.py tests/test_routes_projects_agent_tasks.py tests/test_task_store.py tests/test_routes_tasks.py tests/test_project_task_store.py tests/test_task_lifecycle_notifications.py tests/test_project_events.py tests/projects/test_event_broker_integration.py tests/projects/test_event_broker.py -q
227 passed
Docs-Reviewed: Checklist route docs in docs/agent-coordination.md (lines 837-922)
already match the route surface and remain accurate. The close_task ownership-guard
behavior is merged from dev and covered by changelog.d/2287-close-ownership-guard.md.
The redundant-get_task cleanup changes no API surface.
Files:
changelog.d/2287-close-ownership-guard.md | 3 +
...uby6uh-checklist-event-scope-and-none-safety.md | 3 +
tests/projects/test_task_store.py | 94 ++++++++++++++++++++++
tests/test_routes_projects_agent_tasks.py | 62 +++++++++++++-
tinyagentos/projects/task_store.py | 35 +++++---
tinyagentos/routes/projects.py | 23 ++++--
6 files changed, 202 insertions(+), 18 deletions(-)