fix(tasks): add ownership guard to close_task - #2196
Conversation
📝 WalkthroughWalkthroughChangesTask close authorization
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant CloseRoute
participant ProjectTaskStore
participant TaskDatabase
Caller->>CloseRoute: Request task close
CloseRoute->>ProjectTaskStore: close_task(..., force)
ProjectTaskStore->>TaskDatabase: Update eligible task
TaskDatabase-->>ProjectTaskStore: Success or refusal
ProjectTaskStore-->>CloseRoute: Boolean result
CloseRoute-->>Caller: HTTP response
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
|
|
PR Summary by QodoAdd sponsored agent delegation and task ownership protection
AI Description
Diagram
High-Level Assessment
Files changed (12)
|
Code Review by Qodo
1. Approval replies miss contacts
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| ] | ||
| if denied_scopes: | ||
| question_parts.append( | ||
| f"(Hard-denied: {', '.join(denied_scopes)} — " |
There was a problem hiding this comment.
4. Decision text uses em dash 📜 Skill insight ✧ Quality
The new delegation decision message includes an em dash in text displayed to the approving user. Public-facing text must use a regular hyphen, en dash, or rewritten sentence instead.
Agent Prompt
## Issue description
The delegation decision message contains the prohibited Unicode em dash character.
## Issue Context
Replace the em dash with a regular hyphen, an en dash, or punctuation that preserves readability. Review other added comments, docstrings, and strings in this file for the same character.
## Fix Focus Areas
- tinyagentos/delegation_handler.py[1-8]
- tinyagentos/delegation_handler.py[206-215]
- tinyagentos/delegation_handler.py[563-583]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| invite_store = getattr(request.app.state, "project_invite_store", None) | ||
| if invite_store is None: | ||
| return {"status": "error", "error": "invite store not available"} |
There was a problem hiding this comment.
6. Wrong invite-store state key 🐞 Bug ≡ Correctness
Both delegation approval paths look up app.state.project_invite_store, but application wiring exposes the store as app.state.project_invites. Every delegation approval therefore returns invite store not available without minting an invite.
Agent Prompt
## Issue description
Delegation approval uses an application-state attribute that is never configured, preventing invite minting.
## Issue Context
The application exposes `ProjectInviteStore` as `app.state.project_invites`, matching the existing invite routes.
## Fix Focus Areas
- tinyagentos/delegation_handler.py[264-266]
- tinyagentos/delegation_handler.py[340-342]
- tinyagentos/app.py[1594-1600]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
|
||
| return { | ||
| "status": "approved", | ||
| "invite_id": invite["id"], |
There was a problem hiding this comment.
7. Wrong minted invite key 🐞 Bug ≡ Correctness
The approval helpers read invite["id"], but ProjectInviteStore.mint returns the identifier at invite["record"]["invite_id"]. A successful mint therefore raises KeyError, leaving an orphaned pending invite while approval is reported as failed.
Agent Prompt
## Issue description
Delegation approval reads a nonexistent key from the invite-store result.
## Issue Context
`ProjectInviteStore.mint()` returns `{record: {invite_id: ...}, pin: ...}`.
## Fix Focus Areas
- tinyagentos/delegation_handler.py[294-298]
- tinyagentos/delegation_handler.py[378-382]
- tinyagentos/projects/invite_store.py[241-263]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| "sponsor_contact_id": contact_id, | ||
| "agent_slug": agent_slug, | ||
| "display_name": display_name, | ||
| "pin_required": False, |
There was a problem hiding this comment.
8. Delegation invite pin discarded 🐞 Bug ≡ Correctness
Delegation minting leaves the persisted pin_required argument at its default True and discards the generated PIN, while writing pin_required: False only into inert metadata. Redemption still requires and validates a PIN, so the remote agent cannot redeem the invite using the approval response.
Agent Prompt
## Issue description
Delegation approvals produce invites that require a PIN but never deliver that PIN to the remote contact.
## Issue Context
The metadata flag does not affect the invite store's `pin_required` column or its redeem-time PIN validation. Either securely deliver the generated PIN or implement and enforce a real pinless redemption contract.
## Fix Focus Areas
- tinyagentos/delegation_handler.py[272-298]
- tinyagentos/delegation_handler.py[356-382]
- tinyagentos/projects/invite_store.py[135-142]
- tinyagentos/projects/invite_store.py[369-383]
- tinyagentos/routes/project_invites.py[65-69]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| f"Invite ID: {invite_id}. " | ||
| f"Share this invite with the remote contact so their agent can join." | ||
| ) | ||
| await _route_answer_to_agent(decision, reply) |
There was a problem hiding this comment.
9. Approval replies miss contacts 🐞 Bug ≡ Correctness
Collaboration decisions store the peer contact_id as from_agent, then send approval and denial results through a local A2A helper that drops identifiers not beginning with @. Contact IDs such as hub:hogne consequently receive no decision result or invite credentials.
Agent Prompt
## Issue description
Delegation decision results are routed to the local agent bus instead of the authenticated remote peer.
## Issue Context
Contact identifiers are not required to be local `@agent` addresses. Send a signed peer response/envelope through the contact's peer link, including all required redemption credentials.
## Fix Focus Areas
- tinyagentos/routes/decisions.py[445-480]
- tinyagentos/delegation_handler.py[219-235]
- tinyagentos/routes/decisions.py[45-64]
- tinyagentos/routes/peer.py[168-274]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| all_tasks = await task_store.list_tasks( | ||
| project_id=project_id, | ||
| status="in_progress", | ||
| ) | ||
| tasks = [t for t in all_tasks if t.get("assignee_id") == canonical_id] |
There was a problem hiding this comment.
10. Cascade leaves claimed tasks 🐞 Bug ⛨ Security
The revocation fallback searches for status="in_progress" and matching assignee_id, but this task store represents active claims with status="claimed" and claimed_by. Even selected tasks cannot clear assignment through update_task(..., assignee_id=None) because that method treats None as unchanged.
Agent Prompt
## Issue description
Sponsor revocation does not release tasks held by revoked agents.
## Issue Context
Use the task store's actual claim lifecycle and ownership fields, preferably through an atomic store method that clears `claimed_by` and `claimed_at` and returns the task to `open` only when held by the revoked identity.
## Fix Focus Areas
- tinyagentos/delegation_handler.py[488-524]
- tinyagentos/projects/task_store.py[297-305]
- tinyagentos/projects/task_store.py[307-357]
- tinyagentos/projects/task_store.py[246-292]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| requested_scopes = body["requested_scopes"] | ||
| unknown = sorted(set(requested_scopes) - SPONSORED_DEFAULT_SCOPES - SPONSORED_DENY_SCOPES) |
There was a problem hiding this comment.
12. Malformed scopes crash inbox 🐞 Bug ☼ Reliability
Envelope validation accepts arbitrary values inside requested_scopes, then converts them to a set and sorts them. Unhashable entries or mixed incomparable types raise TypeError, returning a server error after the peer nonce has already been consumed.
Agent Prompt
## Issue description
Malformed scope-list elements can crash delegation request processing instead of producing a validation response.
## Issue Context
The peer route records the signed envelope's nonce before dispatch, so a crash also prevents the sender from correcting and retrying that nonce.
## Fix Focus Areas
- tinyagentos/delegation_handler.py[74-104]
- tinyagentos/routes/peer.py[236-272]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Reviewed at code level, including a red-first proof of the guard tests and an audit of every live caller of the close route. BLOCKED on two structural problems. @hognek fix-forward, but read item 1 first because it changes the branch itself.
What already holds up, so you do not re-litigate it: the guard is store-level SQL, correctly parenthesized, fails closed, agent spoofing is rejected with 403, and there is no new existence oracle (the 409 is only reachable by actors already authorized on the project). Bot adjudication: Qodo 13 (curator/owner cannot force close) is valid and is item 2. Qodo 6 and 7 are valid but target the D1 code riding on the branch; they resolve on #2048. Qodo 1/2/4/8/9/10/12 likewise belong to #2048 and go moot here once the branch is re-cut. CodeRabbit's success status reads "Review rate limited": it reviewed nothing, do not count it. Gitar's pass is a free-plan pass, also not a review. Kilo reported FAIL; read its findings on the re-cut branch before requesting re-review. Re-request review after the re-cut and I will re-verify with the same probes. |
|
Now CONFLICTING after #2194 merged, because this branch still carries #2048's delegation code which collides with the merged decisions.py changes. No new work: the re-cut from origin/dev with only 859afe7 (blocking item 1) resolves the conflict automatically, and the fresh base picks up the merged decisions fixes for free. |
Add claim-holder check to close_task() SQL (AND claimed_by IS NULL OR claimed_by = ?), mirroring the release guard pattern. The route handler passes force=True when the caller is the project lead/curator, letting leads close cards claimed by others. Fixes jaylfc#2191
…audit + route tests Addresses all four blocking items from jaylfc's review on jaylfc#2196: 1. RE-CUT: branch is now clean cherry-pick of 859afe7 onto origin/dev (no jaylfc#2048 contamination) 2. OWNER/ADMIN BYPASS: the route now sets force=True for session owner (project.user_id), session admin (request.state.is_admin), and lead (lead_member_id) — not just the lead. This fixes: - Admin-session sweeper that closes merged cards (was 409 on every claimed card) - Desktop UI: owner closing an agent-claimed card (was 409) 3. CALLER AUDIT: added warning logs on rejected close paths in github_sync.py (both create-then-close and existing-card-close) and beads_bridge.py (close verb). An audit statement in the PR body covers all four callers (sweeper, merge gate, github_sync, beads_bridge). 4. ROUTE-LEVEL TESTS: admin force-close, nonexistent task 404. Store-level tests cover stranger-rejected and lead-force-pass. Proposal 2 (unclaimed cards) kept as-is: forcing claim-first would break github_sync, beads_bridge, the old test suites, and the sweeper. Unclaimed cards remain closeable by any authorized actor, matching the release guard's scope.
859afe7 to
087fbf0
Compare
|
@jaylfc re-cut and addressed all four items:
Proposal 2 (unclaimed cards): kept as-is — forcing claim-first would break github_sync, beads_bridge, the sweeper, and the existing test suites. Unclaimed cards remain closeable by any authorized actor, matching the release guard scope. Ready for re-review. |
![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 (2)
tests/test_board_audit_wiring.py (1)
100-125: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd route-level coverage for the ownership matrix.
These tests cover admin force-close and 404, but not the new 409 stranger rejection or lead/owner force bypasses. Add cases that verify a non-privileged stranger receives
409and leaves the claim intact, plus lead and owner closes of another user’s claimed task.🤖 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_board_audit_wiring.py` around lines 100 - 125, Add route-level tests alongside test_admin_can_force_close_claimed_card covering the ownership matrix: verify a non-privileged stranger receives 409 when closing another user’s claimed task and that the claim remains intact, then add cases confirming lead and owner users can force-close another user’s claimed task successfully. Reuse the existing task setup and assert the relevant status and claim state.tinyagentos/github_sync.py (1)
89-104: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd regression coverage for refused closes.
Test both newly created and existing cards when
close_task()returnsFalse; assert that a warning is emitted andclosedis not incremented.🤖 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/github_sync.py` around lines 89 - 104, Add regression tests for the GitHub sync flow covering both newly created cards and existing cards when task_store.close_task returns False. In each case, assert that the appropriate warning is emitted and the closed counter remains unchanged.
🤖 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/projects/beads_bridge.py`:
- Around line 408-415: Update the refusal handling after _task_store.close_task
in the task-closing flow to log at warning level instead of info, and use
neutral wording that does not assume the task was claimed by another actor.
Preserve the existing identifiers and refusal behavior while removing the
ownership-specific explanation.
In `@tinyagentos/routes/projects.py`:
- Around line 965-968: Update the failed-close handling in close_task to
classify the result from the current row or a structured atomic-update failure
reason, rather than the stale existing pre-read. Treat terminal tasks as
non-ownership failures, and allow forced admin/owner/lead callers to avoid the
"not claimed by you" response; emit that response only when the current task is
non-terminal, non-forced, and owned by someone else.
---
Nitpick comments:
In `@tests/test_board_audit_wiring.py`:
- Around line 100-125: Add route-level tests alongside
test_admin_can_force_close_claimed_card covering the ownership matrix: verify a
non-privileged stranger receives 409 when closing another user’s claimed task
and that the claim remains intact, then add cases confirming lead and owner
users can force-close another user’s claimed task successfully. Reuse the
existing task setup and assert the relevant status and claim state.
In `@tinyagentos/github_sync.py`:
- Around line 89-104: Add regression tests for the GitHub sync flow covering
both newly created cards and existing cards when task_store.close_task returns
False. In each case, assert that the appropriate warning is emitted and the
closed counter remains unchanged.
🪄 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: f2047e65-1bdb-46ba-ba8b-f2711be09e76
📒 Files selected for processing (6)
tests/projects/test_task_store.pytests/test_board_audit_wiring.pytinyagentos/github_sync.pytinyagentos/projects/beads_bridge.pytinyagentos/projects/task_store.pytinyagentos/routes/projects.py
| ok = await self._task_store.close_task( | ||
| tsk_id, closed_by=author, reason=note | ||
| ) | ||
| if not ok: | ||
| logger.info( | ||
| "beads bridge: /%s %s by %s refused (not claimed by closer)", | ||
| verb, tsk_id, author, | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Log refusals as warnings without assuming the cause.
close_task() can return False for an already closed/cancelled or missing task, not only when another actor owns the claim. The current info message is therefore misleading and weaker than the warning-level reporting in tinyagentos/github_sync.py.
Use neutral warning text, or return a structured refusal reason from the store before logging an ownership-specific cause.
Proposed minimal fix
- logger.info(
- "beads bridge: /%s %s by %s refused (not claimed by closer)",
+ logger.warning(
+ "beads bridge: /%s %s by %s refused",
verb, tsk_id, author,
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ok = await self._task_store.close_task( | |
| tsk_id, closed_by=author, reason=note | |
| ) | |
| if not ok: | |
| logger.info( | |
| "beads bridge: /%s %s by %s refused (not claimed by closer)", | |
| verb, tsk_id, author, | |
| ) | |
| ok = await self._task_store.close_task( | |
| tsk_id, closed_by=author, reason=note | |
| ) | |
| if not ok: | |
| logger.warning( | |
| "beads bridge: /%s %s by %s refused", | |
| verb, tsk_id, author, | |
| ) |
🤖 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/projects/beads_bridge.py` around lines 408 - 415, Update the
refusal handling after _task_store.close_task in the task-closing flow to log at
warning level instead of info, and use neutral wording that does not assume the
task was claimed by another actor. Preserve the existing identifiers and refusal
behavior while removing the ownership-specific explanation.
| if not ok: | ||
| if existing.get("claimed_by") and existing["claimed_by"] != closed_by: | ||
| return JSONResponse({"error": "not claimed by you"}, status_code=409) | ||
| return JSONResponse({"error": "cannot close"}, status_code=409) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Classify failed closes from current state, not the stale pre-read.
Because close_task preserves claimed_by after closing, retrying a terminal claimed task can return "not claimed by you" even for an admin/owner/lead using force=True. A claim acquired between the initial read and the atomic update is also misclassified. Re-read the current row (or return a structured failure reason) and emit the ownership response only for a non-terminal, non-forced rejection.
Proposed fix
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:
+ current = await store.get_task(task_id)
+ if (
+ not force
+ and current is not None
+ and current.get("status") not in ("closed", "cancelled")
+ and current.get("claimed_by")
+ and current["claimed_by"] != closed_by
+ ):
return JSONResponse({"error": "not claimed by you"}, status_code=409)
return JSONResponse({"error": "cannot close"}, status_code=409)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if not ok: | |
| if existing.get("claimed_by") and existing["claimed_by"] != closed_by: | |
| return JSONResponse({"error": "not claimed by you"}, status_code=409) | |
| return JSONResponse({"error": "cannot close"}, status_code=409) | |
| ok = await store.close_task(task_id, closed_by=closed_by, reason=payload.reason, force=force) | |
| if not ok: | |
| current = await store.get_task(task_id) | |
| if ( | |
| not force | |
| and current is not None | |
| and current.get("status") not in ("closed", "cancelled") | |
| and current.get("claimed_by") | |
| and current["claimed_by"] != closed_by | |
| ): | |
| return JSONResponse({"error": "not claimed by you"}, status_code=409) | |
| return JSONResponse({"error": "cannot close"}, status_code=409) |
🤖 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 965 - 968, Update the
failed-close handling in close_task to classify the result from the current row
or a structured atomic-update failure reason, rather than the stale existing
pre-read. Treat terminal tasks as non-ownership failures, and allow forced
admin/owner/lead callers to avoid the "not claimed by you" response; emit that
response only when the current task is non-terminal, non-forced, and owned by
someone else.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (6 files)
Reviewed by step-3.7-flash · Input: 183.7K · Output: 30.9K · Cached: 2.4M |
2 participants
Fixes #2191.
Add claim-holder ownership guard to
close_taskin the store, mirroring therelease guard pattern: the SQL now requires
claimed_by IS NULL OR claimed_by = ?.The route handler passes
force=Truewhen the caller is the project lead/curator,allowing leads to close cards claimed by others.
Changes
close_taskgains aforcekwarg. Non-force pathenforces
claimed_by IS NULL OR claimed_by = closed_by.force = project.get("lead_member_id") == actor_id.test_close_by_claimer_passes— claimer closes own claimed cardtest_close_by_stranger_rejected— non-claimer blocked (returns False)test_close_by_lead_passes— lead force-closes another's claimed cardtest_close_unclaimed_unchanged— unclaimed cards still closeable by anyoneTests: 29/29 task_store, 68/68 projects-integration pass.
Summary by Gitar
delegation_handler.pyfor agent delegation requests, scope validation, and sponsor revocationsponsor_contact_idcolumn toagent_registryandproject_inviteswith migration supportpeer.pyanddecisions.pyThis will update automatically on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Tests