fix(invites): allow revoking expired invites, 409 for terminal states#2071
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? |
📝 WalkthroughWalkthroughProject invite revocation now accepts expired invites, rejects redeemed, revoked, or claimed invites with HTTP 409, and reports failed state transitions as conflicts. Store and route tests cover these behaviors. ChangesProject invite revocation
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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}
| row = await store.get(invite_id) | ||
| if row is None or row.get("project_id") != project_id: | ||
| return JSONResponse({"error": "invite not found"}, status_code=404) | ||
| if row.get("status") in ("redeemed", "revoked"): |
There was a problem hiding this comment.
SUGGESTION: Route-level 409 for an already-redeemed invite is untested
The redeemed branch here is only exercised indirectly at the store level (test_revoke_redeemed_invite_returns_false). Add a route test that mints, redeems via store.redeem, then issues DELETE and asserts 409, so this new HTTP path is covered (the PR description claims "revoke-redeemed returns 409" but only the revoked double-revoke path is tested at the route level).
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| ok = await store.revoke(invite_id) | ||
| if not ok: | ||
| return JSONResponse({"error": "invite not found or already redeemed"}, status_code=404) | ||
| return JSONResponse({"error": "invite state changed, retry"}, status_code=409) |
There was a problem hiding this comment.
SUGGESTION: Generic 409 also fires for a transient claimed invite
A mid-redeem invite has status claimed (see invite_store.redeem, which flips pending→claimed). It is not in the ("redeemed", "revoked") guard, so it reaches store.revoke, which returns False (only pending/expired match), landing here with "invite state changed, retry". Behavior is safe (revoke is correctly blocked; a later retry works if the claim rolled back to pending, or returns the explicit 409 once it becomes redeemed), but the message is slightly misleading for that case. Consider adding "claimed" to the explicit guard for clearer observability.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (4 files)
Prior Suggestions — Resolved
Previous Review Summaries (2 snapshots, latest commit 22a9684)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 22a9684)Status: No Issues Found | Recommendation: Merge Files Reviewed (2 files)
Resolved (from prior review)
Previous review (commit 915f977)Status: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (4 files)
Reviewed by hy3:free · Input: 74.1K · Output: 9.3K · Cached: 173.5K |
|
Both Kilo suggestions folded at the new head:
Suite: 73 passed. |
store.revoke only matched status='pending', so an invite that lazily flipped to expired could never be revoked: the route reported 404 while the row kept showing in the pending list. Revoke now covers pending and expired; the route returns 409 with the actual state for redeemed or already-revoked invites instead of a misleading 404. Tests: revoke-expired succeeds (store + route), revoke-redeemed refused, double-revoke 409.
Folds both Kilo suggestions: a 'claimed' invite is mid-redeem, so it now returns a 409 that says so instead of the generic retry text, and both 409 route paths (redeemed and claimed) have direct tests rather than only store-level coverage.
22a9684 to
fb2a1df
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: 1
🤖 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_project_invites.py`:
- Around line 664-668: Update the invite state setup in this test to directly
set the stored invite’s status to redeemed, following the database-update
approach used by the other state-transition tests, instead of calling
store.redeem(). Assert the response’s specific error message in addition to
status code 409 to verify the route exercised the redeemed branch rather than
claimed.
🪄 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: 2064bd83-74c9-4a37-895d-56121258ec0f
📒 Files selected for processing (4)
tests/projects/test_invite_store.pytests/test_routes_project_invites.pytinyagentos/projects/invite_store.pytinyagentos/routes/project_invites.py
🚧 Files skipped from review as they are similar to previous changes (3)
- tinyagentos/projects/invite_store.py
- tinyagentos/routes/project_invites.py
- tests/projects/test_invite_store.py
| body = mint_resp.json() | ||
| store = app.state.project_invites | ||
| await store.redeem(body["invite_id"], body["pin"]) | ||
| resp = await client.delete(f"/api/projects/{pid}/invites/{body['invite_id']}") | ||
| assert resp.status_code == 409, resp.text |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Set status to redeemed to test the correct code path.
Calling store.redeem() only transitions the invite to the claimed state (mid-redeem), based on the store's implementation in tinyagentos/projects/invite_store.py. This causes the test to accidentally hit the claimed branch in the route, which also returns a 409, yielding a false positive for the redeemed state coverage.
To correctly test the redeemed branch, update the database status directly, matching the approach used in the other state-transition tests. Asserting the specific error message will also guarantee the correct branch is exercised.
🛠️ Proposed fix to correctly test the redeemed state
- body = mint_resp.json()
- store = app.state.project_invites
- await store.redeem(body["invite_id"], body["pin"])
- resp = await client.delete(f"/api/projects/{pid}/invites/{body['invite_id']}")
- assert resp.status_code == 409, resp.text
+ iid = mint_resp.json()["invite_id"]
+ store = app.state.project_invites
+ await store._db.execute(
+ "UPDATE project_invites SET status = 'redeemed' WHERE invite_id = ?", (iid,)
+ )
+ await store._db.commit()
+ resp = await client.delete(f"/api/projects/{pid}/invites/{iid}")
+ assert resp.status_code == 409, resp.text
+ assert "already redeemed" in resp.json()["error"]📝 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.
| body = mint_resp.json() | |
| store = app.state.project_invites | |
| await store.redeem(body["invite_id"], body["pin"]) | |
| resp = await client.delete(f"/api/projects/{pid}/invites/{body['invite_id']}") | |
| assert resp.status_code == 409, resp.text | |
| iid = mint_resp.json()["invite_id"] | |
| store = app.state.project_invites | |
| await store._db.execute( | |
| "UPDATE project_invites SET status = 'redeemed' WHERE invite_id = ?", (iid,) | |
| ) | |
| await store._db.commit() | |
| resp = await client.delete(f"/api/projects/{pid}/invites/{iid}") | |
| assert resp.status_code == 409, resp.text | |
| assert "already redeemed" in resp.json()["error"] |
🤖 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_project_invites.py` around lines 664 - 668, Update the
invite state setup in this test to directly set the stored invite’s status to
redeemed, following the database-update approach used by the other
state-transition tests, instead of calling store.redeem(). Assert the response’s
specific error message in addition to status code 409 to verify the route
exercised the redeemed branch rather than claimed.
1 participant
Problem
Live repro from Jay's e2e test: both pending invites hit their 15-minute TTL and lazily flipped to
expiredon read.store.revokeonly matchesstatus='pending', so the DELETE matched zero rows and the route returned 404 "invite not found" - while the dialog kept listing the rows (it labels everything "Pending" regardless of status, tracked in #2065). Net effect: dead invites, displayed as pending, unrevokable.Fix
invite_store.revoke: matchespendingandexpired(an expired invite is exactly what a user wants to clean up).revoke_inviteroute:redeemed/revokednow return 409 with the actual state instead of a misleading 404; 404 remains only for genuinely missing or wrong-project invites.Testing
Related
Summary by CodeRabbit