fix(reef): discover trusted peers as conversations#109905
Conversation
|
Codex review: needs real behavior proof before merge. Reviewed July 17, 2026, 8:17 AM ET / 12:17 UTC. Summary PR surface: Source +484, Tests +598, Docs 0, Other +87. Total +1169 across 25 files. Reproducibility: yes. On the affected main baseline, approve a connected Reef peer with no local session and call conversations_list with channel reef; the persisted-only listing returns no conversation reference, as described by the canonical issue and encoded in the new regression tests. Review metrics: 2 noteworthy metrics.
Stored data model Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance:
Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Keep Gateway-owned directory discovery and address-only persistence with lazy session creation, then land only after rebasing and proving both fresh-peer discovery and correlated two-way Reef turns against the exact head while preserving existing bound conversations. Do we have a high-confidence way to reproduce the issue? Yes. On the affected main baseline, approve a connected Reef peer with no local session and call conversations_list with channel reef; the persisted-only listing returns no conversation reference, as described by the canonical issue and encoded in the new regression tests. Is this the best way to solve the issue? Yes, provisionally. Moving discovery into the credential-owning Gateway, delegating peer enumeration to the channel directory, persisting only the external address, and delaying model-context creation preserves the established session/conversation ownership boundary; exact-head live and upgrade proof remains required. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against db3213264a65. Label changesLabel justifications:
Evidence reviewedPR surface: Source +484, Tests +598, Docs 0, Other +87. Total +1169 across 25 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
Review history (2 earlier review cycles) |
300dac1 to
f26c137
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4b12d5ae16
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 842540b7dd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const route = await params.deps.resolveOutboundSessionRoute({ | ||
| cfg: params.config, | ||
| channel: plugin.id, | ||
| plugin, | ||
| agentId: params.agentId, |
There was a problem hiding this comment.
Skip unroutable directory entries without aborting the list
When a directory entry's route resolver throws, this unguarded await makes the whole conversations.list request fail even though the code already treats null routes as entries to skip. This can happen with configured Google Chat user directory entries that do not yet have a DM space: resolveGoogleChatOutboundSpace throws No Google Chat DM found ..., so one stale/unroutable allowlist user prevents listing every other conversation for that channel. Catch per-entry route errors here and continue/log, matching the existing if (!route) continue behavior.
Useful? React with 👍 / 👎.
842540b to
eb1aade
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eb1aade5bf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
eb1aade to
1960114
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1960114a1f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 230a5017c5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ...(route.peer.kind === "direct" | ||
| ? { nativeDirectUserId: route.peer.id } | ||
| : { nativeChannelId: route.peer.id }), | ||
| ...(display ? { label: display } : {}), |
There was a problem hiding this comment.
Preserve labels when refreshing directory addresses
When a directory entry has no name or handle (for example entries produced by config-backed directory helpers that only return {kind, id}), this registers an identity without a label. upsertConversationIdentity treats a missing label as NULL on conflict, so merely running conversations.list for that channel can erase a label previously learned from inbound/session metadata for the same conversationRef. Preserve the existing label when directory discovery has no display metadata, or only overwrite it when a display value is present.
Useful? React with 👍 / 👎.
|
Land-ready at
Proof:
Known proof gap: the two-host Clawd↔Molty live round trip requires deployment of the landed commit. I will run and report that proof immediately after merge. |
|
Merged via squash.
|
* fix(reef): discover trusted peer conversations * test(reef): document temporary state cleanup * test(gateway): include conversation discovery in method order * chore: leave changelog to release automation * fix(gateway): preserve configured conversation discovery * fix(gateway): validate turns before session binding * fix(gateway): preserve directory conversation identity
Closes #109893
Related: #108666
What Problem This Solves
Fixes an issue where agents using Reef could not discover an approved, connected peer as a conversation until another path had already created a local model-context session for that peer.
Why This Change Was Made
The Gateway now owns external conversation discovery and asks the selected channel plugin for directory entries. Reef exposes locally trusted peers through that directory; listing persists only an address, while the first turn lazily creates and binds the canonical backing session. An optional query keeps handles reachable beyond the default result page. Canonical route identity remains separate from the exact delivery target, so delivery aliases cannot create duplicate conversation references.
This is an additive Gateway protocol change. It does not alter SQLite tables or advance any schema version; the per-agent schema remains version 11.
User Impact
Agents can list and turn to trusted Reef peers on first contact without raw transport targets or a pre-existing local session. Reef stays a transport-backed conversation surface rather than masquerading as a duplicate model session.
Evidence