feat(todo): add agent tools for todo lists, remove notes_set_done#2035
feat(todo): add agent tools for todo lists, remove notes_set_done#2035hognek wants to merge 7 commits into
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
Warning Review limit reached
Next review available in: 28 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 (8)
📝 WalkthroughWalkthroughChangesTodo skill migration
GitHub test fixture
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Agent
participant SkillExecution
participant TodoTools
participant TodoStore
participant TodoNotifications
Agent->>SkillExecution: invoke todo skill
SkillExecution->>TodoTools: delegate operation
TodoTools->>TodoStore: read or update todo data
TodoTools->>TodoNotifications: trigger add-item notifications
TodoTools-->>Agent: return structured result
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
|
|
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
|
|
||
| try: | ||
| store = request.app.state.todo_store | ||
| lists = await store.list_lists(owner_user_id) |
There was a problem hiding this comment.
WARNING: agent_name is never used to authorize the list read — any agent can enumerate another user's todo lists by supplying an arbitrary owner_user_id.
Unlike execute_notes_list_shared_docs (which filters via docs_for_agent(agent_name)), this returns list_lists(owner_user_id) for whatever id the caller passes. There is no check that the calling agent is associated with that owner, so any agent that guesses/learns another user's user_id can read their private lists. This is the same gap flagged across the todo tools (see below). If this is intentional for the "owner-based, no membership yet" phase, please document the trust boundary explicitly and tighten it once C1 collaboration lands.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| return {"error": "list not found"} | ||
| if doc.get("archived_at") is not None: | ||
| return {"error": "list is archived"} | ||
| if doc.get("owner_user_id") != owner_user_id: |
There was a problem hiding this comment.
WARNING: Access gate is doc.get("owner_user_id") != owner_user_id only — the supplied owner_user_id is trusted verbatim, with no binding to agent_name.
Any agent that knows a victim's user_id can add items to that user's list. There is no membership/permission check analogous to notes_tools (agent_members + permission level). The agent_name parameter is validated as a string but otherwise unused for authorization here. Consider binding the agent to a verified owner/tenant context rather than accepting the owner id from the agent payload.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| return {"error": "list not found"} | ||
| if doc.get("archived_at") is not None: | ||
| return {"error": "list is archived"} | ||
| if doc.get("owner_user_id") != owner_user_id: |
There was a problem hiding this comment.
WARNING: Same owner-id-only authorization gap as execute_todo_add_item (line 64). The owner_user_id is caller-supplied and never tied to agent_name, so any agent knowing a user id can toggle completion on that user's items.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| try: | ||
| store = request.app.state.todo_store | ||
| lists = await store.list_lists(owner_user_id) | ||
| return {"lists": lists} |
There was a problem hiding this comment.
SUGGESTION: execute_todo_list_lists returns the full list rows, leaking internal fields (owner_user_id, archived_at, created_at) to the agent. The skill schema description advertises only id, title, updated_at. The notes equivalent (notes_list_shared_docs) explicitly strips owner_user_id (see tests/notes/test_notes_tools.py which asserts owner_user_id not in keys). Consider projecting a safe subset here too.
| return {"lists": lists} | |
| lists = await store.list_lists(owner_user_id) | |
| return {"lists": [{k: l[k] for k in ("id", "title", "updated_at") if k in l} for l in lists]} |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: No new issues found | Recommendation: Merge (pending real CI green at current head) Overview
Scope note — HEAD advanced by dev-branch churn, not PR changesThe PR was rebased onto a new base ( PR #2035's own diff ( Previously flagged issues — status at current HEAD (RESOLVED in code)The three IDOR-style WARNINGs on The owner check also now runs before the archived check in The active inline comments on lines 60/106/164 describe the pre-fix behavior and no longer match current code; they are left in place (not duplicated). Residual note: the no-registry fallback still trusts caller Files Reviewed (PR-owned, 8 files — no new issues)
Previous Review Summaries (7 snapshots, latest commit 43696ce)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 43696ce)Status: No active Kilo issues | Recommendation: Merge after confirming one external note Overview
Incremental diff (2013ae2..43696ce) — RESOLVED by this incrementThe incremental change reorders the access checks in Previously flagged issues — context (unchanged by this increment)The owner-binding IDOR-style warnings on lines 60/106/164 and the internal-field leakage SUGGESTION remain open from prior review rounds but are on lines outside this increment's scope. They are tracked for the Files Reviewed (incremental, 1 file)
Previous review (commit 2013ae2)Status: No active Kilo issues | Recommendation: Merge after confirming one external note Overview
Previously flagged issues — RESOLVED in this incrementThe incremental diff ( Still-open note (flagged by another reviewer, not duplicated here)In Files Reviewed (incremental, 2 files)
Previous review (commit 535b9f0)Status: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (incremental)
Incremental Update NotesSince the previous review ( The core todo files ( Fix these issues in Kilo Cloud Previous review (commit 0c01276)Status: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (2 files in incremental diff)
Incremental Update NotesThe incremental diff (
No new issues introduced. The 3 WARNING authorization gaps remain valid and already have active inline comments (lines 33, 79, 132); no duplicates were posted. The earlier SUGGESTION about leaking internal list fields is resolved (lines 34-39 now project an Fix these issues in Kilo Cloud Previous review (commit f121849)Status: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (1 file in incremental diff)
Incremental Update NotesThe incremental diff (
Fix these issues in Kilo Cloud Previous review (commit 53cf1db)Status: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (1 file in incremental diff)
Incremental Update NotesThe incremental diff (
Fix these issues in Kilo Cloud Previous review (commit 729e473)Status: 4 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (7 files)
Reviewed by hy3:free · Input: 85.4K · Output: 10.3K · Cached: 795.8K |
0c01276 to
535b9f0
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/todo/test_todo_tools.py (1)
42-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an explicit assertion that internal fields are stripped.
This test verifies the allowlisted list response but never asserts that internal fields (
owner_user_id,archived_at,created_at) are absent — the exact contract previously flagged as leaking. The notes-tools equivalent explicitly assertsowner_user_id not in keys; mirroring that here would guard against regression if the allowlist inexecute_todo_list_listsis ever loosened.assert "lists" in res assert any(d["id"] == doc["id"] for d in res["lists"]) assert len(res["lists"]) == 1 + for d in res["lists"]: + assert "owner_user_id" not in d + assert "archived_at" not in d + assert "created_at" not in d🤖 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 `@tests/todo/test_todo_tools.py` around lines 42 - 52, Extend test_list_returns_owned_lists to explicitly verify that each returned list omits the internal fields owner_user_id, archived_at, and created_at, while preserving the existing ownership, inclusion, and count assertions.
🤖 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/tools/todo_tools.py`:
- Around line 71-80: In both execute_todo_add_item and execute_todo_set_done,
perform the owner_user_id authorization check immediately after confirming the
list exists, before evaluating archived_at. Return the existing generic
access-denied error for non-owners, while preserving the archived-list response
for authorized owners.
- Around line 17-41: Bind todo-tool authorization to the calling agent rather
than trusting the LLM-supplied owner_user_id. Update execute_todo_list_lists and
the corresponding add-item and mark-done functions to derive or validate
ownership through the authenticated agent identity and reject mismatches before
accessing TodoStore; preserve owner_user_id only as a validated target when it
matches that identity or an established membership relationship.
---
Nitpick comments:
In `@tests/todo/test_todo_tools.py`:
- Around line 42-52: Extend test_list_returns_owned_lists to explicitly verify
that each returned list omits the internal fields owner_user_id, archived_at,
and created_at, while preserving the existing ownership, inclusion, and count
assertions.
🪄 Autofix (Beta)
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: a0bc0ef9-4eca-4e38-b2f7-ba1480e298c5
📒 Files selected for processing (8)
tests/notes/test_notes_tools.pytests/test_routes_github.pytests/todo/test_todo_tools.pytinyagentos/routes/skill_exec.pytinyagentos/skills.pytinyagentos/todo/notify.pytinyagentos/tools/notes_tools.pytinyagentos/tools/todo_tools.py
💤 Files with no reviewable changes (2)
- tinyagentos/tools/notes_tools.py
- tests/notes/test_notes_tools.py
Create todo_tools.py with execute_todo_list_lists, execute_todo_add_item, and execute_todo_set_done. Register in skill_exec.py SKILL_IMPLEMENTATIONS and skills.py seed data. Remove deprecated execute_notes_set_done from notes_tools.py (set_done now lives on TodoStore). Add tests/todo/test_todo_tools.py (17 tests) covering list/add/set_done with owner-based access control. Remove notes_set_done tests from tests/notes/test_notes_tools.py (11 remaining tests pass). Task: 1923 C3 — Notes/Todo split agent tool surface.
… model Address Kilo bot findings: - Filter owner_user_id/archived_at/created_at from list_lists response - Add docstring notes explaining owner-based authorization model and the planned agent_name-to-owner binding for when TodoStore gains membership No CRITICAL findings. 17/17 todo tools, 11/11 notes tools, 18/18 todo routes pass.
Match the skill schema description which advertises only id, title, updated_at. Using a whitelist (not blacklist) is the safer pattern — new fields added to the store row won't leak through by default.
…ument auth boundary Remove agent_name validation from execute_todo_list_lists and execute_todo_set_done — the parameter was accepted but never used. It was injected by skill_exec.py but served no purpose, creating a false expectation that agent_name participates in authorization. For execute_todo_add_item, agent_name is still required (used for attribution and notification skip-guard). Add explicit SECURITY comments at each owner_user_id check documenting that agent-to-owner binding awaits agent membership on TodoStore (jaylfc#1923 follow-up). The internal field leak in list_lists was already fixed (allowlist projection at line 39).
…eturn private key for github-app-private-key lookups The test helper _build_app_with_app_config(token=None) set mock_secrets.get to return None for ALL keys, causing _get_app_installation_token to fail when looking up the GitHub App private key. Now it returns the key for github-app-private-key lookups while keeping PAT behavior unchanged. Fixes CI failure: test_repo_endpoint_still_uses_app_token (401 vs 200). All 4 tests using _build_app_with_app_config now pass.
Resolve owner_user_id from the agent registry (get_by_handle) instead of trusting the caller-supplied value, closing the authorization gap where any agent knowing a user_id could enumerate, write to, or toggle another user's todo lists. - Add _resolve_owner_user_id() helper: looks up agent_name in agent_registry, returns the verified user_id when registry is available (production), falls back to args-supplied owner_user_id when absent (test compat). - execute_todo_list_lists: requires agent_name, derives owner from registry. - execute_todo_add_item: derives owner from registry instead of trusting args. - execute_todo_set_done: requires agent_name, derives owner from registry. - New tests: registry enforcement (AsyncMock), unregistered agent rejection, fallback without registry. All 21 todo + 11 notes tests pass. Fixes Kilo WARNINGs on PR jaylfc#2035 (agent auth bypass).
… leak Move the owner_user_id gate before the archived_at gate in both execute_todo_add_item and execute_todo_set_done. Previously a non-owner caller who guessed a valid list_id could learn whether the list was archived. Now the owner check runs first, returning a uniform 'access denied' error for non-owners regardless of archived state.
43696ce to
4c89156
Compare
1 participant
Task: #1923 C3 \u2014 Agent tools for Notes/Todo split.
Changes
tinyagentos/tools/todo_tools.pywithexecute_todo_list_lists,execute_todo_add_item,execute_todo_set_donetinyagentos/todo/notify.py\u2014 notification placeholder for when collaboration lands on TodoStoretests/todo/test_todo_tools.py\u2014 17 tests covering list/add/set_done with owner-based access controlexecute_notes_set_donefromnotes_tools.py(set_done now lives on TodoStore)notes_set_donetests fromtests/notes/test_notes_tools.py(11 remaining tests pass)skill_exec.py\u2014 replaced_skill_notes_set_donewith_skill_todo_list_lists/_skill_todo_add_item/_skill_todo_set_doneskills.py\u2014 replacednotes_set_doneskill seed with 3 todo skillsTests
pytest tests/todo/test_todo_tools.py -v --timeout=120\u2014 17/17 passpytest tests/notes/test_notes_tools.py -v --timeout=120\u2014 11/11 passpytest tests/todo/test_todo_routes.py -v --timeout=120\u2014 36/36 pass (no regressions)Design note
TodoStore uses owner-based access control (no agent membership yet). The
owner_user_idarg gates access \u2014 agents must present a matching owner to list/add/set-done. When collaboration lands on TodoStore (C1 follow-up), this will shift to member-based permission checks. The notification path (todo/notify.py) is a no-op placeholder ready for that upgrade.Summary by CodeRabbit
New Features
Changes
Tests