Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4ead461db6
ℹ️ 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".
| /** Kinds the host already reads, folds or routes for channels. */ | ||
| const RESERVED_KINDS = new Set([...CHANNEL_LIVE_KINDS, 39001, 39006]); |
There was a problem hiding this comment.
Reserve every host-owned event kind
Reject all host-owned kinds, not just kinds already present in CHANNEL_LIVE_KINDS. For example, a plugin can currently register workflow kind 30620 or 46020; that makes channelRowKind() return true for host workflow writes, so outbox.send() appends an ms tag, which validateWorkflowEvent() rejects as an unsupported workflow-command tag. Thus activating an otherwise unrelated timeline plugin can break all workflow saves until it unloads. Build this set from all host protocol kinds or explicitly reserve the workflow and other host-owned ranges.
Useful? React with 👍 / 👎.
| 9, 40002, 40008, 45001, 45003, 40099, 40100, 40003, 5, 9005, 7, 39000, 39002, | ||
| 39005, 20002, | ||
| ]; | ||
| const channelKinds = () => [...CHANNEL_LIVE_KINDS, ...pluginRowKinds()]; |
There was a problem hiding this comment.
Refresh established live routes when kinds change
When a plugin registers its kind after a channel route has reached live, mutating pluginKinds does not replace that route: channelKinds() is only evaluated while dispatching a pending route, and update() returns early when the channel IDs are unchanged. Even after the next finite history load exposes existing rows, subsequent events of the registered kind remain absent until a socket reconnect or route recreation. Notify live subscriptions when this registry changes and restart established channel routes with the new filter.
Useful? React with 👍 / 👎.
| const canReact = !!( | ||
| !row.plugin && |
There was a problem hiding this comment.
Hide reporting for plugin timeline rows
Plugin rows suppress reactions here but still satisfy the report predicate below, so deployments supporting kind-1984 reports show a “Report message” action for them. Submitting that dialog always fails because createMessages.report() only accepts isMessageKind() events, while registered plugin kinds are deliberately excluded from that set. Apply the same !row.plugin guard to report availability so the UI does not offer an action the backend always rejects.
Useful? React with 👍 / 👎.
wesbillman
left a comment
There was a problem hiding this comment.
Star Lord automated source review (via Wes’s account)
Four actionable P2 findings are attached inline: broker-backed live delivery, mixed-row read observation, unsupported reporting, and unresolvable copied message links. This is a non-blocking COMMENT review, not an approval or merge authorization.
Reviewed head: 4ead461db6609d41d5936f530c7e69aa9929b622
Pinned base: 3e0a4087b9c09df911f080a3c3a3534d5d7bffab
Reviewed the full 27-file diff and supported registration/disposal, channel read/live/fold/projection, rendering/action, exact-navigation, unread/typing/search/preview callers against the repository contribution and product guidance. The documented next-load behavior for late registration, unsupported-item fallback after unload, trusted in-process plugin model, and plugin-owned relay compatibility are treated as accepted limits, not findings.
Validation limits: source analysis only, using 1,529 hash-verified pinned source files with no dirty source inputs. No PR code, tests, app, relay integration, or CI checks were executed in this review. The author’s validation claims were read but not independently reproduced. Browser/native runtime behavior and deployed-relay compatibility remain unverified.
| 9, 40002, 40008, 45001, 45003, 40099, 40100, 40003, 5, 9005, 7, 39000, 39002, | ||
| 39005, 20002, | ||
| ]; | ||
| const channelKinds = () => [...CHANNEL_LIVE_KINDS, ...pluginRowKinds()]; |
There was a problem hiding this comment.
[P2] Carry registered kinds across the browser-to-broker boundary
The supported app connection uses createCommunities → connectBrokerTransport → subscribeBrokerTraffic (communities/service.ts:99, transport.ts:402–407), while subscribeRelayTraffic runs in the separate Node broker (dev/relay-broker.mjs:1637). Registration updates the browser’s module-local pluginKinds, but the /stream and /stream-interests bodies carry channel IDs, not these kinds (broker-live.ts:91–96,248–253). Consequently this lookup is empty in the broker, so a registered kind such as 40006 can appear in finite history but never arrives on the app’s live channel route—even when registered before opening the channel. This is distinct from the documented late-registration limitation. Pass the validated kind snapshot through the existing per-stream broker contract and use that snapshot for upstream channel filters; cover this actual broker-backed path rather than only the same-process signed transport.
| if (pluginRowKind(event.kind)) { | ||
| rows.push( | ||
| Object.freeze({ | ||
| id: event.id, | ||
| channelId, | ||
| authorId: event.pubkey, | ||
| createdAt: event.created_at, | ||
| createdAtMs: eventMs(event), | ||
| content: event.content, |
There was a problem hiding this comment.
[P2] Exclude plugin rows from viewport read-observation batches
These rows render through MessageRow, whose wrapper still has data-message-id without a non-message marker (MessageRow.tsx:237). use-reading.ts:43–59 therefore includes them among visible message IDs; it excludes only membership rows. After dwell, observe(remained) processes IDs in DOM order, and unread.ts:833 calls requireMessage, which rejects every plugin kind. The first visible plugin row aborts the batch, and the caller silently catches the error, leaving ordinary visible messages below it unread on every subsequent dwell while that row remains visible. Mark plugin rows as ineligible for reading and filter them before submitting the batch, preserving the unread engine’s native-message-only policy. Add mixed plugin/native-row coverage that verifies the native rows still get marked read.
| ? (messageId: string, type: ReportType, note = "") => { | ||
| const original = find(messageId); | ||
| if (!original || ![9, 40002, 40008].includes(original.kind)) | ||
| if (!original || !isMessageKind(original.kind)) |
There was a problem hiding this comment.
[P2] Hide the report action for unsupported plugin rows
This guard correctly rejects non-message kinds, but the new plugin rows still get Report message: MessageRow.tsx:185–196 checks only !row.membership and delivery before exposing session.messages.report. With the normal report-capable session, a user can fill and submit that dialog for a fully loaded plugin row, but every submission rejects here and the dialog says “Failed to submit report. Try again.” Retrying cannot work. Exclude row.plugin from the report action unless reporting these kinds is intentionally supported end to end; test the menu with a report-capable session.
| const AUX = new Set([5, 7, 9005, 40003, 39005, 39006]); | ||
| const PAGE_SIZE = 50; | ||
| const MAX_PAGES = 10; | ||
| const MAX_EVENTS = 2000; |
There was a problem hiding this comment.
[P2] Do not offer message links that cannot resolve plugin rows
Plugin rows still receive messageCopyLink(row, scope) in MessageRow.tsx:330, so both Copy link controls produce a normal buzz://message URL. For a recipient whose current channel window does not contain that row, ChannelsPage.tsx:478–525 falls back to the exact thread reader. That reader rejects the fetched plugin event through this native-only predicate (threads.ts:310–319), reporting the selected message unavailable even with the plugin active and the event readable. Link previews use the same exact reader and also fail. Either suppress Copy link for plugin rows in this timeline-only slice, or deliberately support their exact lookup without enabling replies; cover an off-window target, not just an already-loaded row.
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Re-review: no blocking defects found at 03d6e17b23484acbcadefc01200f5e025d6baab4, against base 3e0a4087b9c09df911f080a3c3a3534d5d7bffab. This is a COMMENT review, not approval.
The previous concrete failures are addressed: broker-backed live kinds propagate and established routes refresh; plugin rows cannot abort read-observation batches or submit unsupported reports/copy links; ordering tags are now confined to core rows, eliminating the workflow-write regression without expanding the reserved-kind list. Reviewed the new unclaimed-row filtering and lifecycle changes as well.
Two optional follow-ups are attached inline: rendered UI regression coverage and resetting replay bookkeeping when replacing channel filters. Neither is a merge criterion.
Validation: source review with independent UI, broker and registration/outbox lanes; full diff and PR-description privacy/artifact inspection; clean diff check. Existing CI passed for this exact head. A focused check using the production session/registry modules verified initial kind synchronization, register/remove updates, and listener cleanup on disposal. No broad suites or live app/relay acceptance were rerun locally.
Separate integration gate: GitHub currently reports merge conflicts. Resolve those and validate the resulting head before merge; this review does not certify a future conflict resolution. The PR description also still describes the old unload placeholder, while the current design and code hide unclaimed rows.
Documentation nit: “host-owned kinds” is broader than the actual reserved channel-route set; narrow that wording rather than adding unrelated reservations.
| ); | ||
| return ( | ||
| <div data-message-id={row.id}> | ||
| <div data-message-id={row.id} data-plugin-row={row.plugin ? "" : undefined}> |
There was a problem hiding this comment.
Optional [P3]: cover the rendered plugin-row guards. The mixed-row dwell regression constructs dataset.pluginRow by hand, so it does not protect this attribute or the report/link wiring. Add a rendered plugin row with a report-capable session to the existing component harness: assert the marker, absence of Report, and disabled Copy link controls, while an ordinary row remains eligible. The implementation is correct on inspection; this is regression coverage, not a blocker.
| wires.delete(route.wire); | ||
| send(["CLOSE", route.wire]); | ||
| delete route.wire; | ||
| route.status = "pending"; |
There was a problem hiding this comment.
Optional [P3]: reset replay bookkeeping on filter replacement. Re-issued routes retain the since from their original creation and the previous count/replay state. Enabling a plugin hours later can therefore replay older retained traffic unnecessarily, and the next EOSE can report a cumulative rather than per-request replay count. Reset these fields as for a fresh route and add a clock-advanced assertion. Replay is capped and finite catch-up still owns recovery; I have not established event loss or a stream-overload failure, so this is non-blocking.
03d6e17 to
2ab41bb
Compare
wesbillman
left a comment
There was a problem hiding this comment.
Star Lord automated source review via Wes’s account
No new actionable source findings in this follow-up. This is a non-blocking COMMENT, not approval or merge authorization.
- Head:
2ab41bb4da6bc10552898717150872b59bf830bf - API base:
ac64f80a4f5e3f699d8de9fafb4329b5466e373d(merge base47ea7e08eb124c5c723afa9a8c3626444b66398b) - Follow-up scope: changes since the feedback-covered
03d6e17b23484acbcadefc01200f5e025d6baab4, prior fixes, and overlapping rebase integration; not a reset of previously agreed exit criteria.
The prior broker-kind propagation, read-observer skip, Report/Copy-link guards, and core-only outbox ordering fixes remain present. The latest repair resets re-issued live channel routes to a fresh replay window/count/retry state while fencing old wire callbacks (live.ts:709–729). The added controlled-clock route regression and rendered MessageRow test cover the two earlier optional follow-ups in the appropriate lower layers. I also traced registration/disposal, fold/render filtering, and preservation of core-only unread/search/typing policies. Mantis’s independent broker/live-route lane returned no findings and has been reconciled. All eight PR commit messages contain DCO sign-offs.
Validation gap: one read-only hosted CI snapshot for this head shows JavaScript and CI required failing. The JavaScript log reports store-discovery.test.ts:148 (“discovers and names 501 same-timestamp memberships…”): expected 128 channel filters, received one; 407 files / 4867 tests passed and one test failed. That job checked merge tree fdacdaf635c2557d364b82f775b92098fd5681e2 (this head + the pinned base); the failing test arrives from the base and is absent from the feature-head archive. This review does not establish its root cause or dismiss the failed gate. Recorded job. The same snapshot shows browser shards, measurements, Rust/tools, security, and DCO successful; Windows skipped.
Source-only review using hash-verified pinned files, with no dirty source inputs. No tests, builds, PR code, app, or live-relay workflows were executed here. Added regression tests were inspected, not run; the PR’s local/human validation claims were not independently reproduced. Native host integration and end-to-end plugin behavior remain unverified by this review.
The set of event kinds the chat timeline treats as messages ([9, 40002, 40008]) was repeated as literals across reads, live routes, fold, store reconciliation, threads, unread, typing, search and hidden DMs, with CHANNEL_ROW_KINDS, CHANNEL_ACTIVITY_KINDS and the live channel route list each restating it. Define it once in relay/kinds.ts and derive the supersets from it. Pure refactor: every filter and predicate matches the same kinds as before. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: murderbot <3754f8729004d95654c46dbab3129e4ab9ef05cc2534e2a3fbfc155983bd637b@buzz.block.builderlab.xyz>
conversation.registerTimelineKind({ id, title, kind, component }) brings
a non-message event kind into channel timelines. The kind joins a
reference-counted plugin set in relay/kinds.ts, so window reads, session
reads, live channel routes and the store's accept filter ask for it; the
fold turns each event into a row carrying `plugin: { kind, tags }` and
the raw content, and the row's body renders through the existing
MessageBody renderer path.
Plugin rows are timeline-only: unread, typing, notifications, search
and the sidebar preview keep using the core message kinds. Reply, react
and edit are hidden on them because the write paths only accept core
message kinds. Kinds the host already folds or routes are refused.
Channels already open pick up a newly registered kind on their next load.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Signed-off-by: murderbot <3754f8729004d95654c46dbab3129e4ab9ef05cc2534e2a3fbfc155983bd637b@buzz.block.builderlab.xyz>
A kind above 65535 made every channel read fail validation. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: murderbot <3754f8729004d95654c46dbab3129e4ab9ef05cc2534e2a3fbfc155983bd637b@buzz.block.builderlab.xyz>
Use scheduled-message kind 40006, which the relay stores per channel, as the test kind instead of an unadmitted one. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: murderbot <3754f8729004d95654c46dbab3129e4ab9ef05cc2534e2a3fbfc155983bd637b@buzz.block.builderlab.xyz>
Derive the reserved plugin kinds from the live channel route list instead of hand-listing them twice, drop a release guard Cordis disposers already give, pass readonly kind constants to filters without copying, and document registerTimelineKind in the plugin architecture. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: drone2 <538af22ae6ba32ed6e86938bbbd536bd128ec0520127cb0f8ecc4661579d51ec@buzz.block.builderlab.xyz>
A plugin row with no active renderer (the plugin unloaded or not yet loaded) showed a generic "Unsupported item" body. The timeline now drops those rows before layout, and registerTimelineKind accepts an optional matches so a plugin can decline rows it cannot render. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: murderbot <3754f8729004d95654c46dbab3129e4ab9ef05cc2534e2a3fbfc155983bd637b@buzz.block.builderlab.xyz>
…uards - Send registered plugin row kinds in the broker's /stream and /stream-interests bodies so the Node broker's upstream routes include them. - Changing the kind set re-issues established channel routes in place. - The outbox adds ordering tags to core row kinds only. - Read marking skips plugin rows instead of aborting the batch. - Plugin rows offer no Report action and no copy link. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: murderbot <3754f8729004d95654c46dbab3129e4ab9ef05cc2534e2a3fbfc155983bd637b@buzz.block.builderlab.xyz>
Add a rendered MessageRow test for plugin rows: the read-skip marker, no Report item, and disabled Copy link, with an ordinary row still eligible. Narrow the docs from "host-owned kinds" to the kinds the host reads or routes for channels, which is what activation rejects. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: murderbot <3754f8729004d95654c46dbab3129e4ab9ef05cc2534e2a3fbfc155983bd637b@buzz.block.builderlab.xyz>
Main now batches joined channels into shared live routes (#359) and renews them make-before-break with replace(). A plugin kind change now uses the same path: established channel and batch routes renew live-only behind their current wire, which closes once the renewal is established, and a route still replaying restarts. Live-only renewals send limit 0, so enabling a plugin never replays old traffic or inflates the replay count; history arrives on the next load as documented. Also keep plugin rows out of the message management menu (#189) and the "mark through" target in the unread badge. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: murderbot <3754f8729004d95654c46dbab3129e4ab9ef05cc2534e2a3fbfc155983bd637b@buzz.block.builderlab.xyz>
2ab41bb to
969023c
Compare
wesbillman
left a comment
There was a problem hiding this comment.
Two changes needed: restore the plugin-row reaction guard (inline) and address the public-material issue below.
- [P2 — public material] The new commit
969023cbexposes an internal relay hostname and agent account identifier in its author/committer email fields. Use the contributors’ verified public-safe contact identities while preserving actual authorship. LegitimateSigned-off-byattribution is explicitly permitted; this finding does not request changing those trailers or substituting the requester as author.
Star Lord’s automated source review via Wes’s account. Head: 969023cb7c94c5e7a87865f1ab863bf0ee652c5d; base: d149d8120f92652a30935740761282edcf1f1a5b (merge base ac002d36). Follow-up to the previous review; prior live/broker fixes remain intact, including Mantis’s reconciled independent lane.
Source-only: pinned Git objects, no dirty source inputs, no code/tests/apps executed. PR description inspected; no attached images. Hosted CI has failing measurements, WebKit shards 4/6 and 5/6, and the required gate; their cause is not established here. Runtime behavior remains unverified. COMMENT only—not approval or merge authorization.
| row.diff || row.plugin ? undefined : parseMediaTimeReply(row.content); | ||
| const displayRow = timeReply ? { ...row, content: timeReply.content } : row; | ||
| const emojiOnly = usesLargeEmojiPresentation(displayRow.content, row.emoji); | ||
| const canReact = !!( |
There was a problem hiding this comment.
[P2] Restore the plugin-row reaction guard lost during the rebase. The previously reviewed 2ab41bb4 gated canReact with !row.plugin, but this head no longer does. In a joined, writable channel with reaction support, a rendered plugin row now gets the quick-reaction buttons and picker (390–404). MessageReactionControls has no additional kind check and calls session.messages.react, whose core-only guard (messages.ts:154–157) always rejects these rows with “Load the message before reacting to it”; retrying cannot succeed. Restore !row.plugin in canReact and extend the rendered-row regression with a reaction-capable session/extensions to assert controls remain available only for ordinary messages.
|
The event-kind consolidation looks useful independently. Which concrete plugin needs non-message timeline events, and what should that experience look like? That would help assess the extension API. If there isn’t a near-term consumer, I’d suggest splitting out the consolidation and deferring the new API. Carl, an automated reviewer, commenting via Wes’s GitHub account. |
Why
There was no single place to change how the chat timeline decides what a message is. The rule "which event kinds are timeline messages" was the literal
[9, 40002, 40008], copy-pasted about 25 times across window fetch, live subscriptions, fold, store reconciliation, threads, unread, typing, search, hidden DMs and reactions. Three near-duplicate supersets sat alongside it. A new kind could not reach a renderer without a dozen edits, and plugins had no way in at all.What
e10b1d6f).src/features/relay/kinds.tsbecomes the single source forMESSAGE_KINDS,CHANNEL_ROW_KINDS,CHANNEL_ACTIVITY_KINDSandMEMBERSHIP_KIND. Every inline copy now uses it. No behavior change. Kind sets that are different policies are left as they are: editable kinds[9, 40002], reaction removal[7, 9, 40002], and the per-site aux lists.73a21f30).ctx.conversation.registerTimelineKind({ id, title, kind, component }). While the plugin is active:ChannelMessagewhosepluginfield carries the kind and tags;MessageBody/registerMessagepath.Reply, react and edit are hidden on plugin rows. They never count toward unread, typing, notification, search or sidebar-preview evidence. Everything is released when the plugin unloads.
h-tagged) kind the target relay stores.108b7e15).CHANNEL_LIVE_KINDS) instead of listed twice.registerTimelineKindis documented indocs/plugin-architecture.md.relay/session.ts,conversation/service.tsxandconversation/contracts.tsare FOUNDATION files. They are edited under explicit direction from @baxen.f0611a5d). The timeline leaves out any plugin row no active renderer claims, including rows folded before the plugin unloaded. There is no placeholder. The optionalmatcheslets a renderer decline individual rows.cb58fd98,2ab41bb4)./streamand/stream-interestsbodies, so live delivery works on the broker-backed path.replace()path main uses for batched routes, so live picks up the kind immediately with no gap and no replay of old traffic. A route still replaying restarts.msordering tags to core row kinds only, so plugin kinds cannot break workflow saves.Known limits.
Validation
47ea7e08:pnpm checkclean and full Vitest 406 files / 4802 tests pass at2ab41bb4.#hroute with 40006 when registered, and drops it when unregistered.live.test.ts: a kind change renews an established singleton route and a batch route live-only with the new kind, keeps the old wires open until the renewals reach EOSE, and restarts a route that is still replaying.MessageRow.test.tsx: a rendered plugin row has the read-skip marker, no Report message and disabled Copy link, while an ordinary row keeps them. Removing either guard fails the test.conversation/service.test.tsx:buzz-relayran in Docker and was driven through the app'sconnectSignedTransport,subscribeRelayTrafficandcreateRelayReader. A real kind 40006 event was published, received on the channel live route, returned by thewindowFilter/parseWindowread, folded into a plugin row, and rendered throughui.Message.buzz-review-completed
🤖 Generated with Claude Code