Task PATCH route rejects the agent registry JWT, so agents cannot edit card bodies/priority - #2240
Task PATCH route rejects the agent registry JWT, so agents cannot edit card bodies/priority#2240jaylfc wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughChangesProject task PATCH authorization
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 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 |
PR Summary by QodoAllow lead agents to PATCH task body/priority with project_tasks JWT
AI Description
Diagram
High-Level Assessment
Files changed (3)
|
|
BOUNCE - the diagnosis is right but the fix breaks a deliberate design law. The change collapses PATCH's required scope from project_tasks_update to project_tasks. project_tasks_update exists precisely so that field-editing is a SEPARATE approval (#2184, beta.45 window): project_tasks was granted to existing agents when it meant read + lifecycle + comments, and the changelog records the principle that 'an existing approval keeps meaning what it meant when it was given'. This PR silently upgrades every existing project_tasks grant to include title/body/labels/priority edits - approval semantics widened after the fact, which is exactly what the split was built to prevent. The new tests then pin the widened behaviour (they mint plain project_tasks and assert edits succeed). The ACTUAL bug behind the card: the seeded internal lead agents carry project_tasks (see _INTERNAL_AGENTS in routes/agent_registry.py) but not project_tasks_update, so the lead cannot edit card bodies even though the route and scope exist for it. That is a grants/seed fix, not a scope collapse. KEEP from this PR: dropping status from _AGENT_EDITABLE_FIELDS (lifecycle routes own status), 400->403 for non-editable fields, and the assignee_id/parent_task_id human-only negatives - those tests are good. FOLD (carded): (1) revert scope back to project_tasks_update on update_task + the middleware comment; (2) add project_tasks_update to the lead's seeded/mint scopes so the card's actual complaint is fixed; (3) keep the new test suite but mint project_tasks_update, and ADD the negative that a plain project_tasks token gets 403 on PATCH - that test is the pin that stops this regression class recurring. |
|
nemotron-ultra-kilo review VERDICT: The changes enable agent PATCH on tasks with proper authorization gates and field whitelisting, but there are missing test cases for rejected fields and a potential over-privilege concern with scope unification.
Automated first-pass review by the nemotron-ultra-kilo lane. The lead still reviews before merge. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@tests/test_routes_projects_agent_tasks.py`:
- Around line 201-229: Add a test alongside
test_agent_patch_assignee_id_rejected and
test_agent_patch_parent_task_id_rejected that creates an agent-authenticated
task PATCH with {"status": "closed"}, then assert the response status is 403.
- Around line 120-200: Add a test alongside the existing PATCH success cases
that creates the task through the agent using project_tasks_create and
project_tasks permissions, without calling set_lead. PATCH the agent-created
task as a non-lead and assert a 200 response with the updated field, covering
the created_by authorization path rather than the lead path.
In `@tinyagentos/routes/projects.py`:
- Around line 919-922: Update the scope description near the project task
endpoint to remove the claim that project_tasks preserves every other label or
is narrower than update_task. State only that this endpoint toggles the
claimable label, while leaving the broader update_task permissions accurately
described elsewhere.
🪄 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: 9ceb2032-a665-49db-b54c-6f462025aa7b
📒 Files selected for processing (3)
tests/test_routes_projects_agent_tasks.pytinyagentos/auth_middleware.pytinyagentos/routes/projects.py
| async def test_lead_agent_patch_body_succeeds(self, ctx): | ||
| """An agent holding project_tasks may PATCH a card body on a board it | ||
| leads -> 200 and the change is reflected in the response.""" | ||
| pid = await _new_project(ctx, "alpha") | ||
| tid = await _new_task(ctx, pid) | ||
| cid, token = await _mint_agent(ctx, pid) | ||
| await ctx.app.state.project_store.add_member(pid, cid, "native") | ||
| await ctx.app.state.project_store.set_lead(pid, cid) | ||
| async with _bare(ctx.app) as bare: | ||
| resp = await bare.patch( | ||
| f"/api/projects/{pid}/tasks/{tid}", | ||
| json={"body": "revised body"}, | ||
| headers=_hdr(token), | ||
| ) | ||
| assert resp.status_code == 200, resp.text | ||
| assert resp.json()["body"] == "revised body" | ||
|
|
||
| async def test_lead_agent_patch_body_persists(self, ctx): | ||
| """The patched body survives a fresh read.""" | ||
| pid = await _new_project(ctx, "alpha") | ||
| tid = await _new_task(ctx, pid) | ||
| cid, token = await _mint_agent(ctx, pid) | ||
| await ctx.app.state.project_store.add_member(pid, cid, "native") | ||
| await ctx.app.state.project_store.set_lead(pid, cid) | ||
| async with _bare(ctx.app) as bare: | ||
| await bare.patch( | ||
| f"/api/projects/{pid}/tasks/{tid}", | ||
| json={"body": "persisted body"}, | ||
| headers=_hdr(token), | ||
| ) | ||
| resp = await bare.get( | ||
| f"/api/projects/{pid}/tasks/{tid}", headers=_hdr(token) | ||
| ) | ||
| assert resp.status_code == 200 | ||
| assert resp.json()["body"] == "persisted body" | ||
|
|
||
| async def test_lead_agent_patch_priority_succeeds(self, ctx): | ||
| pid = await _new_project(ctx, "alpha") | ||
| tid = await _new_task(ctx, pid) | ||
| cid, token = await _mint_agent(ctx, pid) | ||
| await ctx.app.state.project_store.add_member(pid, cid, "native") | ||
| await ctx.app.state.project_store.set_lead(pid, cid) | ||
| async with _bare(ctx.app) as bare: | ||
| resp = await bare.patch( | ||
| f"/api/projects/{pid}/tasks/{tid}", | ||
| json={"priority": 7}, | ||
| headers=_hdr(token), | ||
| ) | ||
| assert resp.status_code == 200, resp.text | ||
| assert resp.json()["priority"] == 7 | ||
|
|
||
| async def test_lead_agent_patch_labels_succeeds(self, ctx): | ||
| pid = await _new_project(ctx, "alpha") | ||
| tid = await _new_task(ctx, pid) | ||
| cid, token = await _mint_agent(ctx, pid) | ||
| await ctx.app.state.project_store.add_member(pid, cid, "native") | ||
| await ctx.app.state.project_store.set_lead(pid, cid) | ||
| async with _bare(ctx.app) as bare: | ||
| resp = await bare.patch( | ||
| f"/api/projects/{pid}/tasks/{tid}", | ||
| json={"labels": ["bug", "urgent"]}, | ||
| headers=_hdr(token), | ||
| ) | ||
| assert resp.status_code == 200, resp.text | ||
| assert resp.json()["labels"] == ["bug", "urgent"] | ||
|
|
||
| async def test_lead_agent_patch_title_succeeds(self, ctx): | ||
| pid = await _new_project(ctx, "alpha") | ||
| tid = await _new_task(ctx, pid) | ||
| cid, token = await _mint_agent(ctx, pid) | ||
| await ctx.app.state.project_store.add_member(pid, cid, "native") | ||
| await ctx.app.state.project_store.set_lead(pid, cid) | ||
| async with _bare(ctx.app) as bare: | ||
| resp = await bare.patch( | ||
| f"/api/projects/{pid}/tasks/{tid}", | ||
| json={"title": "renamed by agent"}, | ||
| headers=_hdr(token), | ||
| ) | ||
| assert resp.status_code == 200, resp.text | ||
| assert resp.json()["title"] == "renamed by agent" | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Test PATCH for an agent-created task.
All successful cases make the agent the project lead. The route also permits a non-lead agent to edit a task where created_by matches the agent. Create a task with project_tasks_create and project_tasks, then PATCH it without assigning the agent as lead.
🤖 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/test_routes_projects_agent_tasks.py` around lines 120 - 200, Add a test
alongside the existing PATCH success cases that creates the task through the
agent using project_tasks_create and project_tasks permissions, without calling
set_lead. PATCH the agent-created task as a non-lead and assert a 200 response
with the updated field, covering the created_by authorization path rather than
the lead path.
| async def test_agent_patch_assignee_id_rejected(self, ctx): | ||
| """assignee_id stays human-only: an agent PATCH of it -> 403.""" | ||
| pid = await _new_project(ctx, "alpha") | ||
| tid = await _new_task(ctx, pid) | ||
| cid, token = await _mint_agent(ctx, pid) | ||
| await ctx.app.state.project_store.add_member(pid, cid, "native") | ||
| await ctx.app.state.project_store.set_lead(pid, cid) | ||
| async with _bare(ctx.app) as bare: | ||
| resp = await bare.patch( | ||
| f"/api/projects/{pid}/tasks/{tid}", | ||
| json={"assignee_id": "someone-else"}, | ||
| headers=_hdr(token), | ||
| ) | ||
| assert resp.status_code == 403 | ||
|
|
||
| async def test_agent_patch_parent_task_id_rejected(self, ctx): | ||
| """parent_task_id stays human-only: an agent PATCH of it -> 403.""" | ||
| pid = await _new_project(ctx, "alpha") | ||
| tid = await _new_task(ctx, pid) | ||
| cid, token = await _mint_agent(ctx, pid) | ||
| await ctx.app.state.project_store.add_member(pid, cid, "native") | ||
| await ctx.app.state.project_store.set_lead(pid, cid) | ||
| async with _bare(ctx.app) as bare: | ||
| resp = await bare.patch( | ||
| f"/api/projects/{pid}/tasks/{tid}", | ||
| json={"parent_task_id": "some-parent"}, | ||
| headers=_hdr(token), | ||
| ) | ||
| assert resp.status_code == 403 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Test that agents cannot PATCH status.
This PR removes status from _AGENT_EDITABLE_FIELDS. These tests reject assignee_id and parent_task_id, but they do not cover the changed status restriction. Add an agent PATCH with {"status": "closed"} and assert HTTP 403.
🤖 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/test_routes_projects_agent_tasks.py` around lines 201 - 229, Add a test
alongside test_agent_patch_assignee_id_rejected and
test_agent_patch_parent_task_id_rejected that creates an agent-authenticated
task PATCH with {"status": "closed"}, then assert the response status is 403.
| is deliberately narrower than PATCH ``update_task``: it toggles ONLY the | ||
| ``claimable`` label and preserves every other label, so granting it to the | ||
| lead agent does not widen the ``project_tasks`` scope beyond a single-label | ||
| toggle. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the project_tasks scope description.
The last sentence is inaccurate. A lead agent with project_tasks can also PATCH title, body, labels, and priority through update_task. State that this endpoint itself only toggles the claimable label.
Proposed fix
- # lead agent does not widen the ``project_tasks`` scope beyond a single-label
- # toggle.
+ # lead agent does not grant an additional mutation path: this endpoint
+ # itself only toggles the ``claimable`` label.🤖 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/projects.py` around lines 919 - 922, Update the scope
description near the project task endpoint to remove the claim that
project_tasks preserves every other label or is narrower than update_task. State
only that this endpoint toggles the claimable label, while leaving the broader
update_task permissions accurately described elsewhere.
Code Review by Qodo
1. Docs omit agent PATCH access
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| auth = await _authorize_task_actor( | ||
| request, pstore, project_id, scope="project_tasks_update" | ||
| request, pstore, project_id, scope="project_tasks" | ||
| ) |
There was a problem hiding this comment.
1. Docs omit agent patch access 📜 Skill insight § Compliance
Agent tokens with the project_tasks scope can now PATCH /api/projects/{pid}/tasks/{id} (behind a
lead/author gate and editable-field allowlist), but in-repo documentation and consent/UI scope
descriptions still characterize project_tasks as read/lifecycle/comments-only and reserve editing
semantics for project_tasks_update. This mismatch can lead operators to grant project_tasks
under the assumption it is non-editing, undermining the allowlist documentation/consent gate and
causing inadvertent over-permission relative to expectations.
Agent Prompt
## Issue description
The repo’s documented/consent semantics for the `project_tasks` scope are stale/inconsistent with the current authorization behavior: agents can now `PATCH /api/projects/{project_id}/tasks/{task_id}` with `scope="project_tasks"` (subject to a lead/authorship gate and a field whitelist), but docs and UI/consent hints still describe `project_tasks` as read + lifecycle + comments only and treat `project_tasks_update` as the editing scope. Align code and all scope descriptions so operators’ understanding of what they grant matches what agents can do.
## Issue Context
- Server-side behavior changed: `update_task` now authorizes agent PATCH using `scope="project_tasks"` and limits what agents can edit via an editable-field whitelist (in addition to a lead/author gate).
- Allowlist/consent documentation has not been updated accordingly: `docs/agent-coordination.md` still documents `project_tasks` as read/lifecycle/comments-only and does not list PATCH under the scope’s allowed routes.
- Other in-repo scope descriptions/UI hints still define `project_tasks` as non-editing and describe `project_tasks_update` as the editing scope, creating a permissions/consent mismatch that can lead to inadvertent over-permission.
- Choose one consistent model and make code + docs/consent text match:
1) Keep separate edit-scope model: require `project_tasks_update` for PATCH again.
2) Adopt widened `project_tasks` model: update all docs/UX hints/tests to reflect that `project_tasks` includes limited PATCH editing; consider deprecating or aliasing `project_tasks_update` to avoid confusing grants.
## Fix Focus Areas
- docs/agent-coordination.md[184-196]
- tinyagentos/routes/projects.py[772-825]
- tinyagentos/routes/agent_auth_requests.py[54-69]
- desktop/src/apps/ProjectsApp/InviteAgentDialog.tsx[4-7]
- desktop/src/apps/agents/AssignAgentToProjectDialog.tsx[6-10]
- tests/test_routes_projects_agent_tasks.py[1-15]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Superseded by #2244 (fold tsk-b6ugu5), which keeps this PR's good parts (status out of the whitelist, 403 semantics, human-only negatives) and fixes the scope collapse. Branch retained until 2244 merges. |
1 participant
CARD TITLE (intent, not commit subject): Task PATCH route rejects the agent registry JWT, so agents cannot edit card bodies/priority
Autonomous build of board card tsk-2bkd56.
Files:
tests/test_routes_projects_agent_tasks.py | 133 ++++++++++++++++++++++++++++--
tinyagentos/auth_middleware.py | 12 +--
tinyagentos/routes/projects.py | 36 ++++----
3 files changed, 153 insertions(+), 28 deletions(-)
Summary by CodeRabbit
claimablelabel.