feat(collab): friend-accept creates contact row + peer link + handshake#2046
feat(collab): friend-accept creates contact row + peer link + handshake#2046hognek wants to merge 1 commit into
Conversation
On hub friend-accept, create the contact row (pinning Ed25519/X25519 pubkeys from the directory response), mint the inbound peer token, fire a background handshake envelope to the peer's endpoints, and record the peer_link. Block cascades to revoke_peer_link. - routes/hub.py: extend accept_friend_request to create contact row, mint peer token, and deliver handshake (fire-and-forget). Extend block_peer to cascade to contacts_store.revoke_peer_link via fingerprint→username lookup through hub_authors. - peer.py: add send_handshake() and deliver_handshake() for building and delivering handshake envelopes to peer endpoints. - tests: 4 new tests — accept creates contact, accept creates peer_link, accept without contacts_store doesn't crash, block cascades to revoke peer_link. All 53 targeted tests pass (hub + contacts + peer). Task: t_a3481cde Closes jaylfc#2014
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? |
|
Warning Review limit reached
Next review available in: 2 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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 (3)
✨ 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}
| if contacts is not None: | ||
| try: | ||
| # Resolve fingerprint → hub_username via the local hub_authors table. | ||
| author = await store.get_author(peer) |
There was a problem hiding this comment.
WARNING: Block cascade will not revoke the peer link in the normal flow.
This relies on store.get_author(peer) (hub_authors by fingerprint) to map the blocked peer fingerprint -> hub_username -> contact_id. But accept_friend_request (the A2 block above) never inserts a hub_authors row for the peer — it only records the friend edge in relationships and creates the contact/peer_link directly from the directory response. So in production get_author(peer) returns None and the revoke_peer_link call is skipped; the peer link silently stays active after a block.
The new test_block_revokes_peer_link only passes because it manually upsert_author("peerFP", username="hogne") first — that step is not performed by the real accept handler, so the test does not reflect production behavior. Either accept_friend_request should persist the peer's fingerprint->username mapping in hub_authors (or another stable lookup), or block_peer should resolve the contact_id via the contact row / relationships edge instead of hub_authors.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| from tinyagentos.hub import identity as _hub_identity | ||
|
|
||
| local_ident = _hub_identity.public_identity() | ||
| from_username = resolve_local_identity_id() |
There was a problem hiding this comment.
WARNING: send_handshake resolves the local identity without a data_dir.
Here resolve_local_identity_id() is called with no data_dir, so it resolves hub.db from TAOS_DATA_DIR env or the default project data/hub path. But the caller in routes/hub.py resolves the request's configured identity via _resolve_local_identity_id(request.app.state.data_dir) (line 394) and intends this same identity to sign the envelope. In any deployment where the hub data dir differs from the default, send_handshake can resolve a different local identity (or raise RuntimeError("cannot send handshake: no local hub identity")), causing the handshake to be built/signed under the wrong identity or fail entirely.
send_handshake should accept a data_dir parameter (or take the already-resolved identity id) and pass it through to resolve_local_identity_id(data_dir).
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
|
|
||
| try: | ||
| for ep in peer_endpoints: | ||
| url = ep.rstrip("/") + "/api/peer/inbox" |
There was a problem hiding this comment.
SUGGESTION: Handshake transmits the plaintext inbound_token secret; enforce TLS and surface delivery failures.
The envelope body carries inbound_token — the secret the peer uses as Bearer to authenticate against our /api/peer/* routes. It is POSTed to the peer's peer_endpoints, which are attacker/peer-supplied and may be plain http://. If a peer endpoint is HTTP, the token travels in cleartext. Consider rejecting/warning on non-HTTPS endpoints, and ensure TLS verification stays on.
Also, the inner except Exception: continue (line 248) silently swallows all errors for every endpoint, so a misconfigured/broken handshake delivery produces no log and always returns False without diagnostics. At minimum, log the failure reason per endpoint so the fire-and-forget task isn't entirely invisible.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (3 files)
Fix these issues in Kilo Cloud Reviewed by hy3:free · Input: 81.8K · Output: 6.9K · Cached: 206.7K |
|
This and #2043 are both the A2 friend-accept slice from sibling branches (feat/collab-a2-friend-accept vs feat/collab-a2-friend-accept-handshake). This one looks like the superset (adds peer.py wiring + the fuller test file). Please close #2043 if this supersedes it, or tell me what #2043 carries that this does not - one PR per slice (pitfall 13). |
|
Three Kilo findings to fold before merge (CI is green and the token-hash storage is right, so these are the only gate):
Also: please close #2043 or state what it carries beyond this PR - still open from my earlier ask. Fold these and A2 merges. |
|
Superseded by #2043. Both PRs implement the A2 friend-accept slice and Kilo flagged the same block-cascade issue on both (the hub_authors lookup returns None because accept_friend_request never caches the peer fingerprint→username mapping, so revoke_peer_link is silently skipped). #2043 has the more comprehensive implementation with 8 tests (vs 4), additional findings to address, and jaylfc's preference. Closing in favor of #2043. |
…gerprint jaylfc deep review at 4b5903b — fold all six findings: 1. BLOCKER: peer_fingerprint retrofit migration was a no-op on every pre-existing DB. BaseStore's migration runner uses baseline-at-latest semantics — existing DBs get stamped at version 1 without executing the ALTER, so the column was absent after init(). The broad except in _try_handshake swallowed the resulting OperationalError, and the block-cascade security fix was similarly swallowed. Replaced the MIGRATIONS list with a guarded _post_init that checks PRAGMA table_info('contacts') and ALTER TABLE ADD COLUMN only when peer_fingerprint is absent. Same pattern as agent_registry_store's _migration_v1_add_status. Fresh databases still get the column from SCHEMA; upgraded databases get it from _post_init. Added two ContactsStore upgrade tests in test_store_upgrades.py following the existing pattern — column-presence check and add_contact-after-upgrade. 2. Fold 1 (send_handshake): A2 intentionally stores the inbound token locally without delivering it — the token exchange channel doesn't exist yet. A3 completes the two-way exchange. The send_handshake envelope builder from jaylfc#2046 is deferred to a follow-up PR linked from the tracking issue. This is a spec deviation from cross-user-collaboration.md Day 0 (mint token on BOTH sides), filed as a tracking issue. 3. Fold 3 (.gitignore): the data/hub/ ignore line is justified — this branch's own history committed identity.json with throwaway test keys at 2b28043 (removed at 500da60). The .gitignore prevents future accidental commits. Squash merge will keep dev history clean. 4. Key hygiene: the keys in 2b28043 were throwaway test keys never used against real endpoints. Squash merge removes them from dev history. jaylfc#2042 re-commits the same file; coordination note added in-thread. 5. Re-trigger: @coderabbitai review after push. 6. Track-don't-block (Kilo W1): accepted the documented NOTE about contact_id bound to peer-controllable username. Follow-up issue filed for fingerprint-keyed contact IDs in a future slice. BONUS: Fixed CodeRabbit nit from head review — test_accept_reupsert_contact now actually revokes between accepts to verify re-establishment clears revoked_at (was a no-op assertion before).
2 participants
A2: Friend-accept → contact row + peer-link handshake
On hub friend-accept, creates the contact row (pinning Ed25519/X25519 pubkeys from the directory response), mints the inbound peer token, fires a background handshake envelope to the peer's endpoints, and records the peer_link. Block cascades to revoke_peer_link.
Changes
accept_friend_requestto create contact row, mint peer token, and deliver handshake (fire-and-forget viaasyncio.create_task). Extendblock_peerto cascade tocontacts_store.revoke_peer_linkvia fingerprint→username lookup through hub_authors.send_handshake()anddeliver_handshake()for building and delivering handshake envelopes to peer endpoints.test_routes_hub_slice3.py— accept creates contact, accept creates peer_link, accept without contacts_store doesn't crash, block cascades to revoke peer link. All 53 targeted tests pass (hub + contacts + peer).Task
Test results
Design notes
asyncio.create_task) — we don't block the HTTP response waiting for the peeroutbound_tokenstarts empty in the initial peer_link; it's filled when the peer responds to our handshakestore.get_author(fingerprint)to resolve peer fingerprint → hub username → contact_id