feat(projects): project lists routes with project-bound agent scope (supersedes #2336) - #2377
Conversation
- Add project_lists to VALID_SCOPES and _ALLOWED_SCOPES - Add _AGENT_LISTS_ROUTES allowlist to auth_middleware - Create project_lists route module with CRUD for lists and entries - Wire project_lists_store and project_list_entries_store onto app.state - Register project_lists router in routes/__init__.py - Init/close lists stores in conftest client fixture - Add tests for owner CRUD, agent with scope, wrong project, missing scope, unauthenticated
Supersedes PR #2336 (branch exec/tsk-epm5m3), whose lane is gone. That branch's route module is merged in here on top of current dev, with the review findings resolved. - reorder logged entry.reordered before checking the store's result, so a reorder that matched no entry returned 400 while the activity feed recorded a successful reorder. Check first, log after. - the reorder body was typed list[dict], so an element missing id or position reached the store and raised KeyError out of the handler instead of failing validation. A ReorderEntryIn model makes it a 422. - project_lists is added to _PROJECT_SCOPES: its routes authorize through check_agent_scope_for_project, which only ever matches a project-bound grant, so approving the scope without a binding would write project_id NULL and leave the agent silently unable to use it. - docs/agent-coordination.md documents the six list routes, the reorder body shape and the 403/404/401 status contract. The 403-vs-404 finding on the original PR no longer reproduces on current dev: a token holding no project_lists grant gets 403 and only a token bound to another project collapses to 404. Evidence in the PR body.
|
ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing |
|
Warning Review limit reached
Next review available in: 37 minutes 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 (1)
📝 WalkthroughWalkthroughAdds a project lists API with list and entry CRUD, entry reordering, project-scoped agent authorization, persistent stores, activity logging, route registration, documentation, and comprehensive authentication and validation tests. ChangesProject lists API
Estimated code review effort: 4 (Complex) | ~45 minutes Mergeability Score: ⚪ Minimal · up to The PR adds project-list routes and improves reorder validation and activity logging; the supplied checks pass, and no actionable merge-blocking risk remains. Sequence Diagram(s)sequenceDiagram
participant Client
participant AuthMiddleware
participant ProjectListsRouter
participant ProjectListsStore
participant ProjectListEntriesStore
Client->>AuthMiddleware: Send authenticated project-list request
AuthMiddleware->>ProjectListsRouter: Pass allowed route to handler
ProjectListsRouter->>ProjectListsStore: Validate project and list access
ProjectListsRouter->>ProjectListEntriesStore: Modify or reorder entries
ProjectListsStore-->>ProjectListsRouter: Return list data
ProjectListEntriesStore-->>ProjectListsRouter: Return entry data
ProjectListsRouter-->>Client: Return HTTP response
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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}
| auth = await _authorize_lists_actor(request, pstore, project_id) | ||
| if isinstance(auth, JSONResponse): | ||
| return auth | ||
| actor_id, _is_agent, _project = auth |
There was a problem hiding this comment.
SUGGESTION: _is_agent is unpacked from the auth tuple but never used in this handler.
The previous code computed author_kind = "agent" if is_agent else "user" here (dead code — store.create_list only accepts created_by, not author_kind). The refactor renamed is_agent to _is_agent to suppress the linter, but the unused tuple element remains. Either remove _is_agent from the unpack, or use it if a future change adds author-kind tracking.
| actor_id, _is_agent, _project = auth | |
| actor_id, _project = auth |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| lst = await lst_store.get_list(list_id) | ||
| if lst is None or lst["project_id"] != project_id: | ||
| return JSONResponse({"error": "list not found"}, status_code=404) | ||
| ok = await store.reorder_entries( |
There was a problem hiding this comment.
WARNING: store.reorder_entries does not validate that each entry belongs to project_id before issuing UPDATEs.
The store method runs UPDATE ... WHERE id = ? AND project_id = ? AND list_id = ?, which silently skips entries that don't belong to the given project/list — it never raises an error or returns per-entry status. The route handler checks the list's project on line 308, but the store method itself has no per-entry project guard. If reorder_entries is ever called without that route-level check (or the list-project guard is refactored away), entries from a different project sharing the same list id could be silently repositioned with no error.
Consider adding an explicit membership check in the store, or at minimum validating each entry's project_id in the route before calling the store.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (1 file)
Previous Review Summary (commit 0ce1efe)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 0ce1efe)Status: 2 Issues Found | Recommendation: Address before merge Overview| Severity | Count | Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (11 files)
Reviewed by step-3.7-flash · Input: 86.8K · Output: 13.6K · Cached: 453.4K |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
tinyagentos/routes/project_lists.py (1)
261-261: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify the
donecoercion.The nested conditional computes three-way logic that
int()expresses directly. The behavior is identical:NonestaysNone,Truebecomes1,Falsebecomes0.♻️ Proposed simplification
- done=1 if payload.done else (0 if payload.done is not None else None), + done=None if payload.done is None else int(payload.done),🤖 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/routes/project_lists.py` at line 261, In the payload update construction, replace the nested conditional assigned to done with direct int coercion while preserving None as None; use the existing payload.done value so True maps to 1, False to 0, and unset values remain None.tests/projects/test_routes_lists.py (2)
242-242: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefix the unused
cidwith an underscore.Ruff reports RUF059 on both lines because
cidis unpacked and never used.test_agent_wrong_project_is_404at Line 313 already uses_for the same value.🧹 Proposed fix
- cid, token = await _mint_agent(ctx, pid) + _cid, token = await _mint_agent(ctx, pid)Also applies to: 274-274
🤖 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/projects/test_routes_lists.py` at line 242, Update the tuple unpacking in the affected test setup lines, including test_agent_wrong_project_is_404’s analogous call, to bind the unused cid value as _ while preserving token usage and existing test behavior.Source: Linters/SAST tools
238-333: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd a session non-owner 404 test.
_authorize_lists_actorintinyagentos/routes/project_lists.pycollapses two distinct cases into an existence-hiding 404: a wrong-project agent token, and a session user who does not own the project.test_agent_wrong_project_is_404pins the first case. No test pins the second case.That path runs through
_get_owned_project, so a future change to the session branch would not be caught here. Add a test that creates a second non-admin session user and asserts 404 onGET /api/projects/{pid}/listsfor a project owned by the admin.🤖 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/projects/test_routes_lists.py` around lines 238 - 333, Add a test alongside TestAgentScopeGating that creates a project as the admin, creates a separate non-admin session user, and sends an unauthenticated-token session request to GET /api/projects/{pid}/lists while authenticated as that user. Assert the response status is 404, covering the _authorize_lists_actor session path through _get_owned_project without changing the existing agent test.
🤖 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.
Nitpick comments:
In `@tests/projects/test_routes_lists.py`:
- Line 242: Update the tuple unpacking in the affected test setup lines,
including test_agent_wrong_project_is_404’s analogous call, to bind the unused
cid value as _ while preserving token usage and existing test behavior.
- Around line 238-333: Add a test alongside TestAgentScopeGating that creates a
project as the admin, creates a separate non-admin session user, and sends an
unauthenticated-token session request to GET /api/projects/{pid}/lists while
authenticated as that user. Assert the response status is 404, covering the
_authorize_lists_actor session path through _get_owned_project without changing
the existing agent test.
In `@tinyagentos/routes/project_lists.py`:
- Line 261: In the payload update construction, replace the nested conditional
assigned to done with direct int coercion while preserving None as None; use the
existing payload.done value so True maps to 1, False to 0, and unset values
remain None.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: bb08fa6a-684b-41e2-8761-2d334c2cd088
📒 Files selected for processing (11)
changelog.d/tsk-wihll4-project-lists-routes.mddocs/agent-coordination.mdtests/conftest.pytests/projects/test_routes_lists.pytests/test_agent_scope_requests.pytinyagentos/app.pytinyagentos/auth_middleware.pytinyagentos/routes/__init__.pytinyagentos/routes/agent_auth_requests.pytinyagentos/routes/agent_registry.pytinyagentos/routes/project_lists.py
|
nemotron-super review VERDICT: Pass Automated first-pass review by the nemotron-super lane. The lead still reviews before merge. |
CodeRabbit was right that nothing covered it, and agent-coordination.md now states the contract, so it needs an assertion behind it. The paired owner test stops the 404 assertion passing for the wrong reason.
|
Bot round adjudicated at head Accepted (1).
Declined with reasons (4).
|
1 participant
Card:
tsk-wihll4. Supersedes #2336 (branchexec/tsk-epm5m3), which had been red and unattended since 2026-08-10 with no lane left to answer the review. STEP 0 of the card was done as instructed:git merge origin/exec/tsk-epm5m3onto current dev, which merged clean (the branch is one commit, andtinyagentos/projects/lists_store.pyitself has been on dev since #2361).Close #2336 as superseded when this merges.
Card item 1 (403 vs 404) is STALE — it does not reproduce on current dev
The card said
tests/projects/test_routes_lists.py:200expects 403 for a missing scope while the code answers 404. On current dev the whole file is green as merged, before any change of mine:The contract the code actually implements is the right one, and it matches
routes/project_notes.py, which this module mirrors:project_listsgrant → 403._verify_agent_scoperaises403 "token does not hold an active 'project_lists' grant", and_authorize_lists_actoronly converts a 403 into a 404 when the detail is exactlyPROJECT_SCOPE_MISMATCH_DETAIL, so this one is re-raised untouched.Rather than take that green at face value, I proved the test is live by widening the collapse to
if exc.status_code == 403:— the case the card claims is happening — and it goes red:So the assertion is sensitive to exactly the defect that was reported, and the defect is not present. No code change for item 1; the file's module docstring said "rejected 401" while the test asserts 403, and that line is corrected to match the verified contract.
What was actually wrong (found in lead review, both red-proven first)
1. A failed reorder logged a successful one.
reorder_entriescalledlog_activity("entry.reordered", ...)before checking the store's return value, then returned 400. An entry id that belongs to no entry in this list rolls the UPDATE back and changes nothing, but the project activity feed recorded a reorder that never happened.2. The reorder body was
list[dict], so a malformed element crashed the store.{"entries": [{"id": "ent-1"}]}reachedProjectListEntriesStore.reorder_entries, which subscriptsentry["position"]—KeyErrorout of the handler instead of a validation error. Now typed asReorderEntryIn(id: str, position: int), so FastAPI answers 422.Both, run against the code as merged, before the fix:
3.
project_listswas missing from_PROJECT_SCOPES. Its routes authorize throughcheck_agent_scope_for_project, which matches only a grant bound to the project. Approving the scope without a binding writesproject_id NULL, and that grant can never match — the operator sees a successful approval and the agent silently has no access. This is the exact failuretest_project_scope_set_is_a_single_definitionwas written about, so that test's expected set is updated deliberately rather than to chase a red.Also removed a dead
author_kindlocal increate_listthat my change to the same function orphaned.Card items 2–4
docs/agent-coordination.mdgains aproject_listsentry documenting the six routes, the reorder body shape, the 403/404/401 contract and the no-activity-on-failure rule. Fragmentchangelog.d/tsk-wihll4-project-lists-routes.md. Not aDocs-Reviewedtrailer.routes/projects.py): the module resolves the project through that file's own_get_owned_projectand keys every route off the project id, like the task routes. Unchanged from the superseded branch; verified, not assumed.Verification
Lead finding filed separately, deliberately not fixed here
project_notesandproject_doc_reviewboth authorize throughcheck_agent_scope_for_projectyet are also absent from_PROJECT_SCOPES— the same silent-no-access defect as item 3, for two scopes that are already shipped. Fixing them changes approval semantics for existing grants, so it gets its own card and its own red-first proof instead of riding along here.Summary by CodeRabbit
New Features
project_listsaccess scope.Documentation