feat(collab): add member_kind=human + collab invite kind + two-sided consent (B1)#2045
Conversation
…consent (B1)
- Widen project_store add_member validation from (native, clone) to
(native, clone, human), accepting contact_id as member_id.
- Add kind (agent|collab), pin_required (bool), and contact_id columns
to project_invites table with boot-time migrations for existing DBs.
- Extend ProjectInviteStore.mint() with kind, pin_required, and
contact_id parameters, plus mark_accepted() for collab response flow.
- Add POST /api/projects/{id}/invites/collab route for minting collab
invites and delivering them as signed envelopes over the peer channel.
- Update peer inbox to dispatch collab_invite envelopes to Decisions
cards (approve_deny type) and handle collab_invite_accept/decline
response envelopes (add member_kind=human on accept, expire on decline).
- Widen projects routes add_member to accept mode=human with
contact_id field.
- Add 12 tests covering human member kind, collab invite minting,
migration safety, and mark_accepted flow.
Reuses jaylfc#2002 invite state machine. Design: docs/design/cross-user-collaboration.md
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? |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughAdds collab invite metadata and delivery, human project membership, peer-based approval handling, invite acceptance lifecycle updates, and coverage for validation, migration, coexistence, and removal behavior. ChangesCollab B1
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant ProjectClient
participant mint_collab_invite
participant contacts_store
participant peer_inbox
participant decision_store
participant project_store
ProjectClient->>mint_collab_invite: Submit collab invite
mint_collab_invite->>contacts_store: Resolve active contact and peer links
mint_collab_invite->>peer_inbox: Deliver collab_invite envelope
peer_inbox->>decision_store: Create approval decision
peer_inbox->>project_store: Add accepted contact as human member
peer_inbox->>project_store: Update invite acceptance or expiration
🚥 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 |
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? |
|
|
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| "error": "project_store not available"} | ||
|
|
||
| try: | ||
| await project_store.add_member( |
There was a problem hiding this comment.
CRITICAL: Accept path adds the authenticated contact to an arbitrary project without verifying the invite.
invite_id and project_id come straight from the signed envelope body, and the only checks are non-empty strings (lines 456-461). There is no lookup of the invite to confirm (a) it exists, (b) its contact_id equals the authenticated contact_id, or (c) it actually belongs to the named project_id. Because the envelope is signed by the contact themselves, any contact can forge a collab_invite_accept body naming any project_id they know and get themselves added as a human member to that project — broken access control / privilege escalation. Add a invite_store.get(invite_id) lookup and assert row["contact_id"] == contact_id and row["project_id"] == project_id before adding the member.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| """ | ||
| UPDATE project_invites | ||
| SET redeemed_by = ?, status = 'redeemed' | ||
| WHERE invite_id = ? AND status = 'claimed' |
There was a problem hiding this comment.
CRITICAL: mark_accepted is a no-op for collab invites — they are never transitioned out of pending.
The UPDATE only matches rows WHERE status = 'claimed' (line 412). But collab invites are minted with status='pending' and the collab acceptance flow (peer _handle_collab_response) never calls redeem(), so they never reach claimed. Consequently mark_accepted matches zero rows for every collab accept, the invite stays pending forever, and list_pending_collab_for_contact keeps returning it as a live pending invite. Either flip pending -> redeemed directly here, or route collab accepts through redeem() first so claimed is reached. As written, accepted collab invites are never marked redeemed.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| "Content-Type": "application/json", | ||
| }, | ||
| ) | ||
| if resp.status_code < 500: |
There was a problem hiding this comment.
WARNING: A non-2xx response < 500 is treated as successful delivery.
if resp.status_code < 500: delivered = True counts any 4xx (e.g. 401/403 bad outbound token, 404 wrong endpoint path) as a successful delivery. The inviter then sees delivered: true while the invitee never received the envelope, and no further endpoints are tried. This both masks delivery failures and can be exploited to spoof delivery. Only treat an explicit success (e.g. resp.status_code == 200 or resp.is_success) as delivered, and keep iterating on any non-2xx.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| try: | ||
| row = await invite_store.get(invite_id) | ||
| if row and row.get("status") == "pending": | ||
| await invite_store._db.execute( |
There was a problem hiding this comment.
WARNING: Decline path reaches into the private invite_store._db instead of using a store method.
Lines 508-512 issue a raw UPDATE project_invites ... SET status='expired' against invite_store._db. This bypasses the store abstraction (and would break if _db were ever wrapped, e.g. for transactions). Add a mark_expired(invite_id) / revoke-style method on ProjectInviteStore and call it here, mirroring mark_accepted. Also note this expires regardless of current status beyond the pending guard, so a claimed/redeemed invite could be force-expired by a forged decline — pair this with the invite-binding check from the accept finding.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| from tinyagentos.peer import build_envelope, resolve_local_identity_id | ||
|
|
||
| # Resolve the local hub identity and the contact's hub username. | ||
| contact_username = contact.get("hub_username", "") |
There was a problem hiding this comment.
SUGGESTION: contact.get("hub_username", "") defaults to an empty string; an empty to_username builds an undeliverable envelope.
If a contact has no hub_username, build_envelope is still called and the envelope is POSTed to the endpoints, where it will fail with a 4xx. Combined with the status_code < 500 delivery check above, this is silently reported as delivered: true. Validate that hub_username is non-empty before building/delivering, and return a clear delivery_error otherwise.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge OverviewIncremental diff since
Files Reviewed (1 file in incremental diff)
Previous Review Summaries (3 snapshots, latest commit 7d3ab82)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 7d3ab82)Status: No Issues Found | Recommendation: Merge OverviewThe incremental diff since commit Files Reviewed (1 file)
Prior findings (not in incremental diff / already resolved)
Previous review (commit d4d2be6)Status: No Issues Found | Recommendation: Merge All 5 prior findings have been resolved by commit
Files Reviewed (4 files)
Previous review (commit da3feb9)Status: 5 Issues Found | Recommendation: Address before merge Overview| Severity | Count | Issue Details (click to expand)CRITICAL
WARNING
SUGGESTION
Files Reviewed (7 files)
Reviewed by hy3:free · Input: 76.3K · Output: 2K · Cached: 160.5K |
CRITICAL fixes: - Accept path now verifies invite exists, is pending, and matches project_id + contact_id before adding member (broken access control). - mark_accepted now accepts both 'pending' and 'claimed' status so collab invites (minted as pending) transition correctly. WARNING fixes: - Delivery status check changed from <500 to 200-299 so 4xx failures are not misreported as success. - Decline path uses new mark_expired() store method instead of reaching into private _db attribute. - Empty hub_username is now guarded before building envelope. Added: mark_expired() store method, updated tests for direct pending→redeemed mark_accepted, and from-claimed path.
![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
🧹 Nitpick comments (1)
tinyagentos/routes/project_invites.py (1)
909-909: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winOffload the blocking
resolve_local_identity_idcall.
resolve_local_identity_idopens a synchronoussqlite3connection and readshub.db, which blocks the event loop when called directly on this async request path.peer_inboxintinyagentos/routes/peer.py(Line 181) already wraps the same call inawait asyncio.to_thread(...); mirror that here for consistency.♻️ Proposed change
- local_id = resolve_local_identity_id(request.app.state.data_dir) + local_id = await asyncio.to_thread( + resolve_local_identity_id, request.app.state.data_dir + )(ensure
asynciois imported in this module)🤖 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_invites.py` at line 909, Update the async request path containing resolve_local_identity_id to call it via await asyncio.to_thread(...), matching the existing peer_inbox pattern. Ensure asyncio is imported in the module and preserve the resulting local_id behavior.
🤖 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/routes/peer.py`:
- Around line 466-494: Validate the invite before mutating membership in the
accepted branch of the peer-inbox handler. Use the project/contact identifiers
to load the pending collab invite, confirm it exists and is addressed to
contact_id, and return the existing received/error response without calling
add_member or log_activity when validation fails. Keep the current membership
mutation only after this authorization guard succeeds.
---
Nitpick comments:
In `@tinyagentos/routes/project_invites.py`:
- Line 909: Update the async request path containing resolve_local_identity_id
to call it via await asyncio.to_thread(...), matching the existing peer_inbox
pattern. Ensure asyncio is imported in the module and preserve the resulting
local_id behavior.
🪄 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: 790ec8a3-d572-43f8-857d-8b3ad84cf190
📒 Files selected for processing (7)
tests/projects/test_invite_store.pytests/projects/test_project_store.pytinyagentos/projects/invite_store.pytinyagentos/projects/project_store.pytinyagentos/routes/peer.pytinyagentos/routes/project_invites.pytinyagentos/routes/projects.py
CodeRabbit nitpick: the blocking sqlite3 call inside resolve_local_identity_id must be offloaded to a thread to avoid blocking the async event loop in the collab invite route handler. Mirrors the pattern already used in routes/peer.py.
443235a to
7d3ab82
Compare
|
Close to mergeable: CI genuinely green on post-fix dev, zero bot findings, the kind whitelist + dual SCHEMA/migration pattern for the new column are both correct, and the #1993 burn/scope protections are untouched. One MUST-FIX before I take it: mint() validates kind against _VALID_KINDS but never constrains scopes for kind=collab. As written, mint(kind="collab", scopes=["a2a_send", ...]) succeeds and the invite row stores those scopes. The locked design (cross-user-collaboration.md, decision 2 + the scope hard-deny) says human collaborators carry NO agent scopes; delegation arrives later via the D-milestone handshake, never on the human invite. Even if the collab redeem path ignores stored scopes today, a scope-carrying collab invite is a footgun for whoever wires redeem next. Fold: in mint(), when kind == "collab" raise ValueError unless scopes is empty, plus a test asserting the rejection (mirror of test_default_kind_is_agent). Pitfall 1 spirit: the constraint plus its negative test land together. Everything else in the slice reads clean. Fix that one gate and I merge on the next pass. |
Collab invites (kind=collab) must carry no agent scopes — delegation arrives later via the D-milestone handshake. Raise ValueError when kind=collab and scopes is non-empty. Fixes jaylfc MUST-FIX on PR jaylfc#2045.
|
Addressed the MUST-FIX: mint() now raises ValueError when kind=collab and scopes is non-empty, with test test_mint_collab_rejects_scopes. All 38 invite_store tests pass. Bots clean on incremental diff. |
2 participants
Summary
Implements milestone B1 from the cross-user collaboration epic (#2012).
Changes
Design
Follows section 4. Reuses #2002 invite state machine (mint/TTL/attempt-cap/claimed). PIN delivered out of band per spec; envelope carries invite_id and project info but not the PIN. Two-sided consent: inviter consents by minting, invitee consents via Decisions card.
Tests
Fixes #2016
Summary by CodeRabbit
humanproject members (add/remove) alongside existing member types.