feat(hub): friend-accept creates contact row and peer-link handshake#2043
feat(hub): friend-accept creates contact row and peer-link handshake#2043hognek wants to merge 6 commits into
Conversation
|
Warning Review limit reached
Next review available in: 45 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 (4)
📝 WalkthroughWalkthroughFriend acceptance resolves peer keys, creates or refreshes local contacts and peer links, and tolerates missing handshake data or storage. Blocking additionally revokes corresponding local peer links and contacts when available. ChangesCollab A2 peer-link lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)Friend acceptance handshakesequenceDiagram
participant Client
participant accept_friend_request
participant Directory
participant ContactsStore
Client->>accept_friend_request: accept friend request
accept_friend_request->>Directory: use directory response and resolve peer keys
accept_friend_request->>ContactsStore: create or refresh contact and inbound peer link
ContactsStore-->>accept_friend_request: persist handshake state
accept_friend_request-->>Client: accepted
Block cascadesequenceDiagram
participant Client
participant block_peer
participant Directory
participant ContactsStore
Client->>block_peer: block peer
block_peer->>Directory: revoke upstream edge
block_peer->>ContactsStore: revoke local peer link and contact
ContactsStore-->>block_peer: update revoked state
block_peer-->>Client: blocked
🚥 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? |
1 similar 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}
| ) | ||
|
|
||
| # Mint the inbound token WE give to the remote instance. | ||
| inbound_token = generate_peer_token() |
There was a problem hiding this comment.
WARNING: Inbound token is minted and stored (hashed) but never delivered to the remote peer
inbound_token = generate_peer_token() is created and persisted via establish_peer_link, but the plaintext is discarded immediately and _try_handshake never returns it or forwards it to the peer (the resp dict passed to _try_handshake is never mutated). The remote instance therefore never learns the token it must present on POST /api/peer/*, so the peer link is non-functional for inbound authentication until a future A3 step. Given this is a security-sensitive auth channel, the handshake should either (a) return the token to the caller so it can be exchanged, or (b) clearly document that A2 intentionally leaves the channel inert and that no peer can authenticate inbound yet. As written, any code path that trusts inbound_token_hash for auth (e.g. find_contact_by_inbound_token) will never match a real remote request.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| try: | ||
| author = await store.get_author(peer) | ||
| if author and author.get("username"): | ||
| await contacts_store.revoke_peer_link(f"hub:{author['username']}") |
There was a problem hiding this comment.
WARNING: Block cascade silently no-ops when the author is absent from hub_authors
The cascade resolves peer → username via store.get_author(peer) (the hub_authors cache). If the contact was created purely from directory pubkeys and the author row was never cached (or was pruned), author is None and the if author and author.get("username") guard skips the revoke entirely. The result: an active peer link remains that can still authenticate on the peer channel after the user blocked the peer. Since block_peer is a strong local-first safety operation, an unrevoked active link is a security gap. Consider falling back to contacts_store.get_contact_by_username/iterating contacts keyed by fingerprint, or at minimum logging a warning when no author is found so the silent skip is observable.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| if contacts_store is None: | ||
| return | ||
|
|
||
| username = directory_resp.get("username") or directory_resp.get("target") or "" |
There was a problem hiding this comment.
SUGGESTION: contact_id is derived from the untrusted directory username
username = directory_resp.get("username") or directory_resp.get("target") comes straight from the remote directory response, then contact_id = f"hub:{username}" is used as the TOFU key-pinning primary key. A peer controls its own username, so a single fingerprint could be pinned under different contact_id values if the name changes between the request and accept flows, fragmenting or shadowing contacts. Binding the contact to the verified peer_fingerprint (e.g. hub:{peer_fingerprint}) and storing username only as a display/lookup column would be more robust against name churn and confusion. Not blocking given the design, but worth considering before this becomes load-bearing.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| inbound_contact = await store.find_contact_by_inbound_token( | ||
| # Generate a new token and use its hash — we can't read the stored plaintext | ||
| # but we can verify the hash is deterministic. | ||
| "placeholder-not-testable-directly" |
There was a problem hiding this comment.
SUGGESTION: Dead code / unused variable left in the test
inbound_contact = await store.find_contact_by_inbound_token(
"placeholder-not-testable-directly"
)
# Actually, we should test the token flow differently.
# Let's just verify the link exists and the hash is consistent.inbound_contact is assigned but never asserted/used, and the "placeholder-not-testable-directly" input is just an arbitrary string that always returns None. This reads like an abandoned attempt to test the token flow. Either remove it, or replace it with a real assertion — e.g. mint a token, call establish_peer_link with it, then assert find_contact_by_inbound_token(token) returns the contact, proving the hash lookup actually works.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (5 files)
Resolved since previous review (incremental PATCH 6, commit 948dbad)
Carried-forward open issues (unchanged by this incremental diff)
Incremental review scopeOnly PATCH 6 (commit 948dbad, since 4b5903b) was within scope: Previous Review Summaries (3 snapshots, latest commit 4b5903b)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 4b5903b)Status: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (3 files)
Resolved since previous review (commit a0a07fd+)
Carried-forward open issues (unchanged by this incremental diff)
No NEW issues were introduced by the incremental changes — the added fingerprint guard and Previous review (commit 0254a6e)Status: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (3 files)
Resolved since previous review (commit a0a07fd+)
Carried-forward open issues (unchanged by this incremental diff)
No NEW issues were introduced by the incremental changes — the added fingerprint guard and Previous review (commit 8feef8d)Status: 4 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
SUGGESTION
Files Reviewed (3 files)
Reviewed by hy3:free · Input: 135.5K · Output: 9.4K · Cached: 462.9K |
df2155e to
fb637cf
Compare
| @@ -0,0 +1 @@ | |||
| {"signing_private": "9af62cb1e50222bd57e894e3dab5f9444851f0309224c0b37bf45ed21143d178", "signing_public": "9ae572677819955cf2aace721129e8a1d393d21ecc47ef964db8c3f0ff3b98da", "encryption_private": "c02cc57f5a4940700d9a80dd4578467e63308a251bf643f26c13c3f049670662", "encryption_public": "a1ac687647b24884912035b4c67ae749f136477a1bb031d24817a66236717005", "created_at": 1784503810.4560094} No newline at end of file | |||
There was a problem hiding this comment.
CRITICAL: Committed private signing + encryption keys in plaintext
This file contains real private key material (signing_private, encryption_private) and is tracked by git (not in .gitignore). Private keys must NEVER be committed — once pushed they persist in history and are compromised. Rotate these keys immediately and remove the file from the branch and history (e.g. git filter-repo / BFG). The identity is generated at runtime (0600) by tinyagentos/hub/identity.py; do not seed it via a committed file.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| # Can't form a contact_id without a username. | ||
| return | ||
|
|
||
| contact_id = f"hub:{username}" |
There was a problem hiding this comment.
WARNING: contact_id derived from untrusted directory username enables TOFU key-pinning confusion
contact_id = f"hub:{username}" uses username taken from the directory response (directory_resp.get("username") or ...get("target")), which is attacker-influenceable. A malicious or compromised directory can pin the peer's public keys under an arbitrary/colliding hub:<name>, colliding with an existing local contact or impersonating a different user. The trust-on-first-use pin should be keyed by the verified peer fingerprint (hub:{peer_fingerprint}), which is already available, not a directory-supplied display name. This also roots the dead block-fallback below (see line 509).
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| # Fallback: scan contacts for a matching ed25519 fingerprint. | ||
| all_contacts = await contacts_store.list_contacts() | ||
| for contact in all_contacts: | ||
| if contact.get("ed25519_pub") == peer: |
There was a problem hiding this comment.
SUGGESTION: Block fallback compares pubkey to fingerprint — effectively dead code
The fallback scans contacts and matches contact.get("ed25519_pub") == peer, but peer is the signing fingerprint (a hash, e.g. deadbeef…) whereas ed25519_pub is the raw public key (ab…). The contacts table has no fingerprint column (see contacts_store.py:14). These encodings never match, so this branch always falls through to the "could not resolve" warning — the exact no-op the PATCH 2/2 fallback was meant to fix. Store the peer fingerprint on the contact (e.g. a peer_fingerprint column) and match on that, or resolve via hub_authors before scanning.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| # outbound token) completes the two-way exchange. Until then, the | ||
| # inbound auth channel is inert (no remote request will carry this | ||
| # token) and find_contact_by_inbound_token() will never match. | ||
| inbound_token = generate_peer_token() |
There was a problem hiding this comment.
SUGGESTION: Peer link created in a non-functional/inert state
The inbound token is minted and stored but never delivered to the remote peer (no exchange channel yet), and outbound_token is an empty placeholder — so find_contact_by_inbound_token() can never match and the link cannot authenticate inbound requests until A3. The row existing in active/established state may be mistaken for a ready channel by future code (or monitoring). Consider tracking an explicit handshake state (e.g. pending/awaiting_reply) so the inert row is distinguishable from a live peer link.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@data/hub/identity.json`:
- Line 1: Remove the committed runtime identity artifact at
data/hub/identity.json, add data/hub/*.json (or the equivalent specific ignore
rule) to .gitignore, and treat the exposed signing and encryption keys as
compromised by rotating them if they were used against a real hub.
In `@tests/test_collab_a2_handshake.py`:
- Around line 225-235: Close each manually created HubStore after its
upsert_author call completes to prevent leaked database connections: add the
cleanup at tests/test_collab_a2_handshake.py lines 225-235 and 398-408,
preferably using try/finally so HubStore.close runs even if upsert_author fails.
In `@tinyagentos/routes/hub.py`:
- Around line 499-520: The fallback scan in the hub block flow should compare
the peer signing-key fingerprint with a fingerprint derived from each contact’s
ed25519_pub, not the raw public-key value. Update the loop around
contacts_store.list_contacts and revoke_peer_link to use the existing
fingerprint derivation utility, preserving the matching contact_id revocation
behavior, and add coverage for the missing or stale hub_authors fallback path.
- Around line 119-142: Before saving contact keys in _try_handshake, compute the
fingerprint of ed25519_pub with identity.fingerprint and compare it to
peer_fingerprint. If they differ, log a warning identifying the contact and skip
the handshake; only continue to add the contact when the fingerprints match.
🪄 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: d4d3c999-8a91-4629-b44b-730514716980
📒 Files selected for processing (3)
data/hub/identity.jsontests/test_collab_a2_handshake.pytinyagentos/routes/hub.py
|
Good use of the supersede convention on #2046, but the survivor is missing half the slice. #2046 carried tinyagentos/peer.py (the send_handshake sender side); this PR only has the hub.py receive side plus tests. Before this merges as THE A2 slice:
CI is green and the PR is mergeable, but I am holding until 1 and 3 are answered and the applicable parts of 2 are folded or explicitly relocated. |
On friend-accept: - Extract peer Ed25519/X25519 pubkeys from directory response - Fall back to hub_authors cache when directory omits pubkeys - Create contact row (trust-on-first-use key pinning) - Mint inbound peer token (hashed at rest) - Establish peer link with advertised endpoints - Handshake is best-effort — failures never block the accept On block: - Cascade to contacts_store.revoke_peer_link() - Resolve fingerprint->username via hub_authors cache Tests: 8/8 pass (contact creation, pubkey fallback, no-pubkey skip, endpoint parsing, re-upsert, missing-store guard, block cascade, block cascade missing-store). Existing 37 contacts_peer tests unaffected. Part of jaylfc#2012 (cross-user collaboration), milestone A2. Closes jaylfc#2014.
…doc, dead test code - WARNING: block cascade now falls back to contact-table scan when hub_authors cache is missing, with explicit log warning on failure - WARNING: document that A2 intentionally stores inbound token locally without delivering it (A3 completes the exchange) - SUGGESTION: remove dead test code (placeholder token lookup)
…allback, doc contact_id - CRITICAL: remove committed data/hub/identity.json (test-generated keys) and add data/hub/ to .gitignore - WARNING: document that contact_id is derived from untrusted directory username (TOFU key-pinning bound to peer-controllable name) with future direction - SUGGESTION: remove broken fingerprint-vs-pubkey fallback in block cascade (peer fingerprint != ed25519_pub key — comparison would never match)
…erification - SUGGESTION: wrap HubStore init/upsert in try/finally with close() in both test_collab_a2_handshake.py locations to prevent leaked database connections - SUGGESTION: verify directory-supplied ed25519_pub fingerprint matches expected peer_fingerprint in _try_handshake; skip handshake on mismatch to avoid pinning TOFU keys from an imposter - Update _PEER_FP test constant to actual fingerprint of _PEER_SIGNING_PUB so the new fingerprint check passes consistently Tests: 103/103 pass (collab A2 handshake + hub + contacts peer)
e1076db to
0254a6e
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
tinyagentos/routes/hub.py (1)
136-139: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor: local variable
hub_storeshadows the module-levelhub_storeimport.
hub_store = await _get_store(request)rebinds the name also used as a module reference elsewhere in this file (e.g. line 93'shub_store.HubStore(...)). Not a bug today (not reused after), but risks confusion if the function grows. Consider naming itstorefor consistency withblock_peer's usage.🤖 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/hub.py` around lines 136 - 139, The fallback branch in the relevant route shadows the module-level hub_store import. Rename the local result of _get_store(request) to store and update the subsequent get_author call, matching block_peer’s naming without changing behavior.tests/test_collab_a2_handshake.py (1)
307-346: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winRevocation-clearing assertion doesn't test revocation-clearing.
The comment claims
revoked_at is Noneproves "re-establish clears revocation" (Line 346), but the link is never revoked between the two accept calls —revoked_atwas alreadyNonebeforehand, so this passes regardless of whether re-establish actually clears revocation.🧪 Suggested fix to actually exercise the clearing behavior
store = app_with_contacts.state.contacts_store first_link = await store.get_peer_link(f"hub:{_PEER_USERNAME}") first_established = first_link["established_at"] + # Simulate a prior revocation to verify re-establish actually clears it. + await store.revoke_peer_link(f"hub:{_PEER_USERNAME}") + revoked_link = await store.get_peer_link(f"hub:{_PEER_USERNAME}") + assert revoked_link["revoked_at"] is not None + # Second accept with different endpoints — should update dir_resp_body["endpoints"] = ["https://second.example.com:6969"](Adjust the revoke call to whatever method
contacts_storeactually exposes.)🤖 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_collab_a2_handshake.py` around lines 307 - 346, Update test_accept_reupsert_contact to revoke the stored peer link after the first accept and before the second accept, using the contacts_store revocation method exposed by the implementation. Assert the link is revoked before re-accepting, then retain the existing second-accept assertions to verify re-establishment clears revoked_at and refreshes the endpoints.
🤖 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_collab_a2_handshake.py`:
- Around line 381-458: Add a test alongside
test_block_cascades_to_contacts_store that creates the contact and peer link but
omits the HubStore.upsert_author setup, then posts to /api/hub/friends/block
using the peer fingerprint. Assert the request succeeds with blocked state and
verify get_peer_link reports revoked_at and get_contact reports status
"revoked", covering the handler’s fallback username resolution.
In `@tinyagentos/routes/hub.py`:
- Around line 511-533: Update the block cascade around contacts_store and
revoke_peer_link to resolve the blocked peer from a signing-fingerprint value
persisted on the contacts row, rather than relying solely on store.get_author
and hub_authors. Add the required contacts-store schema, persistence, and lookup
support, revoke the matching peer link even when no hub_authors entry exists,
and add coverage for blocking a peer that was never upserted as a contact.
- Around line 133-160: Widen the exception boundary in _try_handshake to cover
the hub_authors lookup, public-key validation, and identity.fingerprint check,
not only the handshake side effect. Ensure malformed directory keys or database
lookup failures are caught and logged, then return without propagating an
exception so accept_friend_request preserves its successful-response guarantee.
---
Nitpick comments:
In `@tests/test_collab_a2_handshake.py`:
- Around line 307-346: Update test_accept_reupsert_contact to revoke the stored
peer link after the first accept and before the second accept, using the
contacts_store revocation method exposed by the implementation. Assert the link
is revoked before re-accepting, then retain the existing second-accept
assertions to verify re-establishment clears revoked_at and refreshes the
endpoints.
In `@tinyagentos/routes/hub.py`:
- Around line 136-139: The fallback branch in the relevant route shadows the
module-level hub_store import. Rename the local result of _get_store(request) to
store and update the subsequent get_author call, matching block_peer’s naming
without changing 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: 0d2de826-ccbe-4cfd-8abd-365ad5ef0d0c
📒 Files selected for processing (3)
.gitignoretests/test_collab_a2_handshake.pytinyagentos/routes/hub.py
🚧 Files skipped from review as they are similar to previous changes (1)
- .gitignore
…ade fingerprint fallback - Widen try/except in _try_handshake to cover hub_authors lookup, fingerprint validation, and endpoint processing — prevents ValueError from bytes.fromhex() on malformed directory pubkeys from crashing the accept endpoint (CodeRabbit CRITICAL). - Add peer_fingerprint column to contacts table with migration, store it at friend-accept for stable fingerprint→contact lookup. - Implement fingerprint-based fallback in block_peer's contact cascade: when hub_authors is missing or stale, resolve via get_contact_by_fingerprint() instead of silently skipping. - Rename hub_store→store in _try_handshake to avoid shadowing the module-level import (CodeRabbit nit). - Add test_block_cascade_fingerprint_fallback: verifies block revokes peer link via fingerprint when hub_authors is empty.
|
Deep review at head 4b5903b. Good movement: the fingerprint-fallback block cascade, widened exception boundary, endpoint normalization, and the 9 new tests are all solid, and most earlier bot findings are genuinely fixed at head. But this is a HOLD on one reproduced upgrade-breaking defect plus two unanswered folds:
Item 1 is the gate. Everything else can land in the same push. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== PR metadata and changed files =='
gh pr view 2043 --repo jaylfc/taOS --json number,headRefOid,body,commits,files \
--jq '{number, headRefOid, body, commits: [.commits[] | {oid: .oid, messageHeadline: .messageHeadline}], files: [.files[].path]}'
echo
echo '== Relevant file locations =='
fd -a -t f '^(contacts_store|db_migrations|knowledge_store|peer|hub)\.py$|^cross-user-collaboration\.md$|^\.gitignore$|^test.*collab.*|^test.*contact.*' . \
| sed -n '1,160p'
echo
echo '== Structural outlines =='
for f in $(fd -a -t f '^(contacts_store|db_migrations|knowledge_store|peer|hub)\.py$' .); do
echo "--- $f"
ast-grep outline "$f" --items all --view compact || true
done
echo
echo '== Migration, handshake, and contact references =='
rg -n -C 4 \
-e 'MIGRATIONS' \
-e '_post_init' \
-e 'migration_v1_add_user_id' \
-e 'peer_fingerprint' \
-e 'get_contact_by_fingerprint' \
-e 'send_handshake' \
-e 'resolve_local_identity_id' \
-e 'inbound_token' \
-e 'data/hub' \
--glob '*.py' --glob '*.md' --glob '.gitignore' . \
| sed -n '1,500p'
echo
echo '== Relevant commit file lists =='
for sha in 2b28043a 500da607 4b5903b3; do
echo "--- $sha"
git show --no-ext-diff --format='%H %s' --name-status "$sha" 2>&1 | sed -n '1,100p' || true
doneLength of output: 42728 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="jaylfc/taOS"
echo '== ContactsStore schema, migration, and lifecycle =='
contacts=$(fd -a -t f '^contacts_store\.py$' . | head -n1)
dbm=$(fd -a -t f '^db_migrations\.py$' . | head -n1)
ks=$(fd -a -t f '^knowledge_store\.py$' . | head -n1)
tests=$(fd -a -t f '^test_collab_a2_handshake\.py$' . | head -n1)
peer=$(fd -a -t f '^peer\.py$' . | rg '/tinyagentos/peer\.py$' | head -n1)
sed -n '1,260p' "$contacts"
echo '--- db_migrations ---'
sed -n '1,240p' "$dbm"
echo '--- analogous guarded migration ---'
sed -n '90,145p' "$ks"
echo '== A2 tests relevant to legacy upgrades and block cascade =='
rg -n -C 5 \
-e 'upgrade|existing|legacy|peer_fingerprint|never.*upsert|block_cascade' \
"$tests" tests \
| sed -n '1,360p'
echo '== Sender implementation definitions/call sites =='
rg -n -P -C 3 '\b(send_handshake|build_envelope|resolve_local_identity_id)\s*\(' \
"$peer" tinyagentos tests \
| sed -n '1,360p'
echo '== Design specification: A2/A3 handshake statements =='
design=$(fd -a -t f '^cross-user-collaboration\.md$' . | head -n1)
rg -n -i -C 4 'day 0|A2|A3|handshake|token' "$design" | sed -n '1,360p'
echo '== Existing GitHub issues/PRs mentioning sender-side handshake or fingerprint-keyed contacts =='
gh search issues \
--repo "$repo" --state open \
--match title,body \
--limit 100 \
--json number,title,url,state \
'send_handshake OR "fingerprint-keyed" OR "contact_id" OR "peer handshake"' \
--jq '.[] | {number,title,url,state}' || trueLength of output: 76029
Required follow-up before re-review
The remaining holds also remain valid:
After the migration fix and regression test are pushed, re-trigger 🐇🔍 ✏️ Learnings added
|
…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).
|
Thanks for the thorough deep review. All six findings folded at 948dbad: 1. BLOCKER — fixed. Replaced the 2. Fold 1 (send_handshake) — deferred. A2 intentionally stores the inbound token locally without delivering it (the token exchange channel doesn't exist yet). The 3. Fold 3 (.gitignore) — answered. The 4. Key hygiene — confirmed. The keys at 2b28043 were throwaway test keys never used against real endpoints. Squash merge removes them from dev history. Re #2042: coordination note added — when #2042 lands on dev, the 5. CodeRabbit — re-triggered. @coderabbitai review 6. Track-don't-block (Kilo W1) — accepted. The documented NOTE about BONUS: re-upsert test now actually revokes between accepts so the |
|
I’ll review the current head to verify the implementation and regression coverage. 🐇✅ ✏️ Learnings added
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! ✅ Action performedReview finished.
|
2 participants
On friend-accept, create the contact row (pin Ed25519/X25519 pubkeys), mint the inbound peer token, exchange endpoints, and record the peer_link. Subscribe to friend block/revoke → cascade.
Changes
tinyagentos/routes/hub.py: Added_try_handshake()helper that extracts pubkeys from the directory response (with hub_authors fallback), creates a contact row, mints an inbound token, and establishes a peer link. Called fromaccept_friend_requestafter the hub relationship is recorded.block_peernow cascades tocontacts_store.revoke_peer_link().tests/test_collab_a2_handshake.py: 8 new integration tests covering the full accept→contact+link flow and block→revoke cascade.Design decisions
hub_authorstable (populated during friend-request flow)hub_authorscacheTests
test_contacts_peer.pytests pass (no regressions)Part of #2012 (cross-user collaboration), milestone A2.
Closes #2014.
Summary by CodeRabbit