Conversation
There was a problem hiding this comment.
🔁 This review has been superseded. See the latest review.
There was a problem hiding this comment.
🔁 This review has been superseded. See the latest review.
|
TODO: Verify the complete pairing flow against real channel credentials, including request creation, Web approval and private-chat code verification, success notification, and delivery of a new message after pairing. Live Telegram guidance has been observed; the full end-to-end flow has not yet been verified. |
There was a problem hiding this comment.
🔁 This review has been superseded. See the latest review.
There was a problem hiding this comment.
🔁 This review has been superseded. See the latest review.
|
Documentation follow-up: the public Telegram setup guide does not explain group privacy mode, which group messages Telegram delivers, or how group permissions affect reception and replies. It also describes the old pairing flow.
Live testing found that explicit bot commands produce a group pairing request and guidance after the local classification fix. Ordinary mentions, including native mention selection, produce no adapter update with this test bot's privacy mode enabled. Full group acceptance remains in progress. The current GitHub account cannot access the private amantru/rome-cloud repository referenced by Rome's docs, and no local website source was found. Website edits need source access. Track this here for now; split it into an issue if separate ownership or scheduling is needed. |
There was a problem hiding this comment.
🔁 This review has been superseded. See the latest review.
There was a problem hiding this comment.
🔁 This review has been superseded. See the latest review.
168b634 to
3527c3a
Compare
There was a problem hiding this comment.
🔁 This review has been superseded. See the latest review.
There was a problem hiding this comment.
🔁 This review has been superseded. See the latest review.
There was a problem hiding this comment.
🔁 This review has been superseded. See the latest review.
There was a problem hiding this comment.
🔁 This review has been superseded. See the latest review.
zoolsher
left a comment
There was a problem hiding this comment.
Code Review: 🛑 REQUEST_CHANGES
This PR substantially consolidates Telegram, Discord, and Feishu account authorization around the existing approval records, with shared validation, transactional mapping writes, authenticated Web resolution, bounded request creation, and expanded UI and lifecycle coverage. The overall direction is coherent, but the admission path is not serialized with connection teardown, and the approval response remains coupled to an unbounded best-effort provider notification.
Verdict: REQUEST_CHANGES — A teardown race can leave a stale pairing request that still grants guardian authority after its connection or Talk grant has been removed.
2 finding(s) posted as inline comments below.
| Severity | Category | File | Title |
|---|---|---|---|
| P1 | security | packages/core/src/channels/pairing.ts |
Pairing admission can race connection teardown |
| P2 | error-handling | packages/core/src/api/routes/approvals.ts |
Best-effort notification can indefinitely delay approval response |
zoolsher
left a comment
There was a problem hiding this comment.
Code Review: 🛑 REQUEST_CHANGES
This PR cleanly separates provider credential setup from guardian authorization and unifies Telegram, Discord, and Feishu pairing through the approval system, with strong identity binding, audit history, authenticated Web resolution, and broad regression coverage.
Two lifecycle issues remain: admission is not serialized with connection teardown, and best-effort provider notification remains on the approval response path without a deadline. Targeted tests could not run in the provided clone because dependencies were not installed.
Verdict: REQUEST_CHANGES — Connection teardown can race pairing admission and leave an approvable request for a disconnected channel.
2 finding(s) posted as inline comments below.
| Severity | Category | File | Title |
|---|---|---|---|
| P1 | security | packages/core/src/channels/pairing.ts |
Pairing admission can race connection teardown |
| P2 | error-handling | packages/core/src/api/routes/approvals.ts |
Best-effort notification can indefinitely delay approval response |
# Conflicts: # packages/web/src/pages/ActivityPage.tsx
What this PR does
Guardian account pairing is coupled to channel credential setup, with separate verification flows and a Discord first-sender mapping fallback. This PR separates credential setup from account authorization for Telegram, Discord, and Feishu. It routes account pairing through the existing
person_mappingapprovals.Closes #181, following the account-pairing direction in the issue discussion.
Pairing flow
Settings→Connections, with aPairing Guidelink.account-id) is paired with Rome. You can start chatting now.” The provider resolves the native mention. A delivery failure does not undo approval.Shared approval surfaces
Activity(/activity) provides approval controls and audit history.Connections(/settings/connections) is the active channel-pairing subset of the same records, with the same resolver and confirmation behavior.Implementation and review repairs
guardianresolver. Dashboard access alone does not grant approval authority.PersonMappingRepositoryinside the approval transaction.Design & Invariants
Deliberately retained behavior
Web rejection ends the current request without granting account access. Connections removes it from the active list; Activity retains the rejected record, decision time, and guardian identity. The request's verification code becomes invalid immediately. The bot sends no rejection notification in either a group or a private chat.
Rejection preserves the request's original expiry as a cooldown. Until then, further messages from that account create no new request and send no further pairing guidance. For example, a request created at 18:12 and rejected at 18:15 remains in cooldown until 18:22. After expiry, a new eligible message can create a fresh request; expiry alone does not create one. Rejection is not a permanent account ban. Automated tests cover rejection and cooldown. The user also exercised Web rejection during live testing. A full elapsed-time live cooldown test is not claimed.
Security and audit
RP-plus eight hexadecimal characters, derived from a private profile key. They are scoped to one request and compared after trimming and case normalization.Contracts: channels.md and access-control.md.
Test plan
Verified
pnpm typecheckin the Nix devShell.pnpm test:unitrun passed Core 4,370, Web 1,399, and UI 559 tests, plus the other workspace groups.pnpm typecheckpassed and Core passed 4,374 unit tests. The targeted pairing suite passed 19 tests.pnpm dev:alland confirmRome started.Live Telegram acceptance
Rome omitted explicit bot commands from directed-message classification. The fix has a regression test and a real group test:
/start@<test-bot>created a request and delivered guidance. Temporary diagnostic logs have been removed.With the test bot's group privacy enabled, ordinary mentions, including native mention selection, produced no adapter update. At the user's authorization, group privacy was disabled through BotFather on September 9 at 10:31 UTC. An ordinary mention then reached Rome and produced guidance. No group administrator grant or remove/re-add was needed in this test. The setting remains disabled. The Telegram FAQ describes privacy-mode delivery restrictions.
Verified live on September 9 (UTC):
The user confirmed Web approval, private-code approval, and normal conversation after pairing. Repository records confirm Web resolution at 10:21:56 and code resolution at 10:23:43.
A group-origin request was created from an ordinary mention at 10:31:55. At 10:34:53 its code reached Rome from the group, but the request remained pending, failed attempts stayed at zero, and the account remained unpaired. The bot returned pairing guidance; the original expiry was unchanged.
The requesting account sent the same code in private at 10:35:36. The request resolved by verification_code, linked the account, and retained zero failed attempts. Native Telegram showed the success notification with the check emoji, username, and ID. Connections removed the pending request.
At 10:36, an ordinary group mention after pairing reached the agent and received a normal reply in the group, confirmed in native Telegram.
At 10:40:31, an ordinary group mention created a fresh request. The browser approved it at 10:40:55 (Web guardian dev). The request had no stored private conversation ID, so notification used the directMessaging fallback. Native Telegram confirmed the private success notification, and Connections removed the request.
Telegram acceptance is complete for the agreed scope: real approval/delivery paths plus automated shared security and lifecycle checks. The final targeted run passed 56 tests across pairing, Telegram adapter, and approval API routes. These cover identity/channel/connection binding, code replay, five-error lockout with Web approval, rejection cooldown, expiry, and approval races. Cross-account and time/lockout edges have automated coverage, not a claimed second-account or elapsed-time live test. Discord and Feishu acceptance results appear below. Test resets retained approval history and only reset the designated Telegram account/request state. The test account is left paired; group privacy remains disabled as authorized.
Live Discord acceptance
The user waived the live DM-disabled case. Automated tests cover best-effort notification failure without approval rollback. No successful delivery is claimed when the provider refuses private messages.
Live Feishu acceptance
Only the designated Feishu account mapping was reset between cases. Approval history was retained, and the earlier request expired naturally before the group case. Test accounts remain paired.
The live matrix covers the three channels supported by this PR's account-pairing admission. WhatsApp and WeChat retain provider-owned setup flows. This is not an acceptance claim for every Connections integration.
Live acceptance screenshots
Captured from the September 9 local acceptance runs. Personal identifiers, avatars, identity badges, and historical verification codes are masked. Unrelated sidebars are cropped. These are redacted screenshots of actual conversations, not mockups. They show the local changes described above, now included in the pushed branch.
Telegram: private and group evidence
Private code redemption and success notifications; group guidance and post-pairing AI conversation. The group image includes separate test reset cycles.
Discord: private and group evidence
Private success notification after Web approval; group pairing guidance and the subsequent AI thread preview.
Feishu: private and group evidence
Private-code success, AI reply, and the later group-origin approval notification; group guidance and post-approval AI reply.
Remaining checks and review items
9dae23f.Delivery and CI status
The pushed head is
9dae23f, includingb8e691d. It contains the accepted card, copy, spacing, shorter-code, Telegram command, native-mention, and teardown changes. The September 9 screenshots capture the live acceptance before the final review repairs. Subsequent automated tests and browser checks cover those repairs.Canonical main was merged at
8323855in commitf7387dc. On that merge, one core CI run reported an uncaughtECONNRESETin an unchanged relay test while all assertions passed. Targeted local tests and the failed-job rerun passed without a code change. The exact emitting socket was not established.An earlier check of the pushed head showed successful CI results, including integration and E2E. That observation is historical, not a fresh CI check for this description update. Final-head CI validation remains pending. An earlier local full-integration run failed a live Codex app-server model call while its other 30 tests passed. The complete local full-integration suite has not been revalidated on the final head.
Not in this PR