Carry OS-owned objective CHECKLIST forward onto dev (1 conflicting file) -- supersedes PR #2415 - #2473
Carry OS-owned objective CHECKLIST forward onto dev (1 conflicting file) -- supersedes PR #2415#2473jaylfc wants to merge 1 commit 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.
|
ⓘ 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: 36 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 (7)
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 |
|
nemotron-super review VERDICT: Pass Automated first-pass review by the nemotron-super lane. The lead still reviews before merge. |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| row = await cur.fetchone() | ||
| desc = cur.description | ||
| item = _row_to_checklist_item(row, desc) | ||
| await self._publish(task_id, "checklist.item.created", {"id": item["id"], "text": item["text"], "task_id": task_id}) |
There was a problem hiding this comment.
WARNING: _publish(task_id, ...) passes task_id where project_id is expected
The _publish method signature is _publish(self, project_id, kind, payload). Every other call in this file passes project_id, but here task_id is passed instead. This causes checklist.item.created events to be published under the task ID rather than the project ID, so subscribers listening on the project channel will miss them.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| done: bool | None = None, | ||
| verified: bool | None = None, | ||
| reported: bool | None = None, | ||
| ) -> dict: |
There was a problem hiding this comment.
WARNING: Return type dict is incorrect — method can return None
update_checklist_item calls get_checklist_item which returns dict | None. When no candidates are provided (line 785) or the item does not exist, the method returns None, violating the -> dict type hint. Callers that assume a dict is always returned will crash with TypeError.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| verification or a report. | ||
| """ | ||
| item = await self.get_checklist_item(item_id) | ||
| if item["verified"] != 1: |
There was a problem hiding this comment.
WARNING: Missing None check before accessing item["verified"]
get_checklist_item returns dict | None. If item_id does not exist, item will be None and item["verified"] raises TypeError: 'NoneType' object is not subscriptable. The method should check if item is None before accessing fields, and raise a ValueError (or let the caller handle it) for a missing item.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| (now, item_id), | ||
| ) | ||
| await self._db.commit() | ||
| await self._publish(item["task_id"], "checklist.item.archived", {"id": item_id, "task_id": item["task_id"], "archived": True}) |
There was a problem hiding this comment.
WARNING: _publish(item["task_id"], ...) passes task_id where project_id is expected
Same issue as line 749: _publish expects project_id as its first argument, but item["task_id"] is passed. This causes checklist.item.archived events to be published under the task ID rather than the project ID.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| # Checklist routes | ||
| # --------------------------------------------------------------------------- | ||
|
|
||
| class CreateChecklistItemIn(BaseModel): |
There was a problem hiding this comment.
SUGGESTION: CreateChecklistItemIn missing _TaskRequestModelMixin pattern
All other task input models in this file (CreateTaskIn, UpdateTaskIn, ClaimIn, etc.) use _TaskRequestModelMixin, which logs unknown keys and uses extra="allow". CreateChecklistItemIn has no model config, so extra fields are silently ignored (Pydantic default extra="ignore"). This is inconsistent with the established pattern and can mask client bugs.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| if isinstance(guard, JSONResponse): | ||
| return guard | ||
| t = await store.get_task(task_id) | ||
| if t is None or t["project_id"] != project_id: |
There was a problem hiding this comment.
SUGGESTION: Redundant get_task call after _require_task_in_project
_require_task_in_project already fetches the task and validates it belongs to the project (line 1278-1280). The subsequent get_task call on line 1282 repeats the same DB query for a task whose existence and project membership have already been confirmed. The route can reuse the guard return value directly.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 6 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: 122.9K · Output: 19.4K · Cached: 359K |
|
Closing superseded by card tsk-uby6uh (BASE: exec/tsk-gzwv3x — carry content preserved). Two reasons: (1) branch went CONFLICTING after today's merges; (2) Kilo's four WARNINGs are all REAL, verified against d3156e8: |
1 participant
CARD TITLE (intent, not commit subject): Carry OS-owned objective CHECKLIST forward onto dev (1 conflicting file) -- supersedes PR #2415
Autonomous build of board card tsk-gzwv3x.
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):
(from auth: allowlist agent Bearer access to task checklist routes #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 OS-owned objective CHECKLIST (agent cannot silently drop items) #2415 checklist
section (list/create shapes, 404 existence-hiding, activity-feed logging,
archive rules) immediately before "## Answering a select decision".
(refused 401 at the allowlist, pinned by two strict xfails). dev's allowlist
already matches the checklist paths (per auth: allowlist agent Bearer access to task checklist routes #2430), so carrying OS-owned objective CHECKLIST (agent cannot silently drop items) #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.
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.
Files:
changelog.d/tsk-gzwv3x-task-checklist-items.md | 2 +
docs/agent-coordination.md | 30 +++-
tests/projects/test_task_store.py | 83 +++++++++++
tests/test_routes_task_checklist.py | 191 +++++++++++++++++++++++++
tinyagentos/projects/ids.py | 2 +-
tinyagentos/projects/task_store.py | 128 +++++++++++++++++
tinyagentos/routes/projects.py | 73 ++++++++++
7 files changed, 506 insertions(+), 3 deletions(-)