fix(broker): scope observer links to a minted token; resync on replay_gap#391
Conversation
…_gap "Join as observer" links previously embedded the full Relaycast workspace API key (rk_live_...) — the same credential used to spawn/write to the workspace — so sharing an observer link handed out full workspace control. Add an IPC round trip (broker:mint-observer-token) that POSTs directly to the broker's own local /api/observer-token endpoint (mirroring the existing getClientApiKey/getClientBaseUrl pattern for direct broker HTTP calls) to mint a scoped, read-only ot_live_... token instead, cached per project so repeated clicks don't spam the workspace with unrevoked tokens. The renderer "Join as observer" action is now async, with minting/error UI states. Separately, the broker's dashboard WebSocket can send a `replay_gap` frame when its replay buffer can't cover a reconnect (e.g. a burst of worker_stream chunks evicted an undelivered relay_inbound chat message). Nothing previously handled this kind, so a dropped message vanished silently. Recognize replay_gap in the shared event schema, log it in broker.ts's ingress pipeline, and have agent-store.ts trigger the existing message-reconciliation flow (use-message-reconciliation.ts) to resync the active room's canonical history — reusing the current debounced reconcileMessages pipeline rather than building a parallel one. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 1 minute 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: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (13)
✨ 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces scoped, read-only Relaycast observer tokens to replace the full workspace API key for "Join as observer" links, implementing token minting, caching, and an updated UI with loading and error states. It also adds support for handling replay_gap events to trigger a debounced resync of active room history when a reconnect cannot be fully covered by the replay buffer. The feedback suggests adding a React key to the observer link component to prevent stale state, configuring a timeout on the token minting fetch request to avoid hanging indefinitely, and moving global stub cleanups to an afterEach block to prevent test pollution.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| value={compactValue(workspaceKey)} | ||
| copyValue={workspaceKey} | ||
| action={observerUrl ? <ObserverLink href={observerUrl} /> : undefined} | ||
| action={workspaceKey ? <ObserverLinkAction projectId={broker.projectId} /> : undefined} |
There was a problem hiding this comment.
If the workspace key changes (e.g., if the user joins a different workspace or the broker is restarted with a new key) while the project ID remains the same, the ObserverLinkAction component will retain its stale 'ready' state with the old observer URL/token. This is because React preserves component state when the component type and position in the tree remain the same.
By adding key={workspaceKey} to ObserverLinkAction, React will automatically unmount the old component and mount a fresh one in the 'idle' state whenever the workspace key changes, preventing users from opening or copying stale observer links.
| action={workspaceKey ? <ObserverLinkAction projectId={broker.projectId} /> : undefined} | |
| action={workspaceKey ? <ObserverLinkAction key={workspaceKey} projectId={broker.projectId} /> : undefined} |
There was a problem hiding this comment.
Confirmed — this was a real bug. ObserverLinkAction had no key, so React preserved its 'ready' state (with the old minted URL) across re-renders when the project's workspaceKey changed underneath it (rejoin/restart with the same projectId). handleClick's if (state.status === 'ready') { window.open(state.url, ...); return } short-circuit meant it would never re-mint in that case — it'd just keep reopening the stale token's URL.
Fixed in 4ca7bd6: added key={workspaceKey} to the <ObserverLinkAction> element, so a workspace-key change now remounts the component fresh (back to 'idle'), forcing a new mint on the next click.
| response = await fetch(`${baseUrl}/api/observer-token`, { | ||
| method: 'POST', | ||
| headers, | ||
| body: JSON.stringify(body) | ||
| }) |
There was a problem hiding this comment.
The fetch request to the local broker does not have a timeout configured. If the local broker is unresponsive or wedged, this request will hang indefinitely, keeping the IPC promise pending and leaving the UI in a permanent "Minting" state.
Adding a timeout using AbortSignal.timeout(5000) ensures the request fails gracefully after a reasonable duration.
response = await fetch(`${baseUrl}/api/observer-token`, {
method: 'POST',
headers,
body: JSON.stringify(body),
signal: AbortSignal.timeout(5000)
})There was a problem hiding this comment.
Confirmed valid — the fetch had no timeout, so a wedged/unresponsive broker would hang the mintObserverToken IPC promise (and the renderer's "Minting" state) indefinitely.
Fixed in 4ca7bd6: added a dedicated OBSERVER_TOKEN_REQUEST_TIMEOUT_MS = 5_000 constant and passed signal: AbortSignal.timeout(OBSERVER_TOKEN_REQUEST_TIMEOUT_MS) on the fetch call. A timeout now surfaces through the existing catch (err) block as a normal rejected promise (wrapped via toErrorMessage), same as any other network failure, so the UI's existing error state handles it without extra plumbing.
| json: async () => ({ token: 'ot_live_abc', id: 'obs-1' }), | ||
| text: async () => '' | ||
| })) | ||
| vi.stubGlobal('fetch', fetchMock) |
There was a problem hiding this comment.
Stubbing fetch globally inside individual tests without a try...finally block or a global afterEach cleanup can lead to test pollution. If any assertion fails before vi.unstubAllGlobals() is reached at the end of the test, the global fetch stub will leak into subsequent tests, causing cascading and hard-to-debug failures.
Consider adding vi.unstubAllGlobals() to the file's global afterEach block to ensure cleanups always run regardless of test outcomes.
There was a problem hiding this comment.
Confirmed valid. The per-test vi.unstubAllGlobals() calls at the end of each mintObserverToken test body would indeed get skipped if an earlier assertion in that test threw, leaking the stubbed fetch into subsequent tests.
Fixed in 4ca7bd6: moved vi.unstubAllGlobals() into the enclosing describe('BrokerManager local + cloud coexistence', ...) block's existing afterEach (which already runs unconditionally, pass or fail) and removed the redundant per-test calls.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4bb4011674
ℹ️ 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".
|
|
||
| let response: Response | ||
| try { | ||
| response = await fetch(`${baseUrl}/api/observer-token`, { |
There was a problem hiding this comment.
This POST goes to /api/observer-token, but the broker binary bundled by this repo is still the locked @agent-relay/broker-* 9.1.6 dependency, and that version's local API router does not register an observer-token route. In normal packaged or npm ci installs, pressing Join therefore replaces the old working workspace-key link with a request that returns 404, so observer links cannot be generated unless the broker dependency is bumped to a version with this endpoint or a fallback path is kept.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, this is expected — already called out in the PR description: this PR's IPC/UI plumbing depends on a parallel relay repo PR that adds the /api/observer-token endpoint to the broker. Until that lands and Pear's @agent-relay/broker-*/@agent-relay/harness-driver pin is bumped to a version that registers this route, the request will 404 at runtime. Not a bug in this PR — it's the expected cross-repo sequencing, tracked as a follow-up dependency rather than something to work around here.
- ObserverLinkAction: key on workspaceKey so a rejoin/restart that changes the underlying workspace (same projectId) remounts the component fresh instead of keeping a stale 'ready' state showing the old minted token/URL. - mintObserverToken: add a 5s AbortSignal.timeout to the direct fetch, so a wedged/unresponsive broker can't hang the IPC promise (and the renderer's "Minting" UI) indefinitely. - broker.test.ts: move the mintObserverToken tests' `vi.unstubAllGlobals()` fetch-stub cleanup into the describe block's existing afterEach, so a failed assertion earlier in a test can't leak the stub into later tests. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found and verified against the latest diff
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/renderer/src/components/broker/BrokerDetailsPage.tsx">
<violation number="1" location="src/renderer/src/components/broker/BrokerDetailsPage.tsx:441">
P2: The `window.open` after minting succeeds runs outside the original click handler's gesture context (after `await`), so it may be blocked by popup blockers in configurations where `nativeWindowOpen: true` or in test/web contexts. The 'ready' state fast-path on line 434 is fine since it's synchronous. Consider opening the window before the `await` (e.g., with `about:blank` and updating the location), or accepting this trade-off since Electron's default renderer doesn't enforce popup blocking.</violation>
</file>
<file name="src/main/broker.test.ts">
<violation number="1" location="src/main/broker.test.ts:845">
P2: Global `fetch` stubs in these tests rely on `vi.unstubAllGlobals()` at the end of the test body, but if an assertion fails before that line, the stub leaks into subsequent tests causing cascading failures. Consider adding `vi.unstubAllGlobals()` to the file-level `afterEach` (or wrapping in `try...finally`) to ensure cleanup always runs regardless of test outcome.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| const result = await pear.broker.mintObserverToken(projectId) | ||
| const url = observerUrlForToken(result.token) | ||
| setState({ status: 'ready', url }) | ||
| window.open(url, '_blank', 'noreferrer') |
There was a problem hiding this comment.
P2: The window.open after minting succeeds runs outside the original click handler's gesture context (after await), so it may be blocked by popup blockers in configurations where nativeWindowOpen: true or in test/web contexts. The 'ready' state fast-path on line 434 is fine since it's synchronous. Consider opening the window before the await (e.g., with about:blank and updating the location), or accepting this trade-off since Electron's default renderer doesn't enforce popup blocking.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/renderer/src/components/broker/BrokerDetailsPage.tsx, line 441:
<comment>The `window.open` after minting succeeds runs outside the original click handler's gesture context (after `await`), so it may be blocked by popup blockers in configurations where `nativeWindowOpen: true` or in test/web contexts. The 'ready' state fast-path on line 434 is fine since it's synchronous. Consider opening the window before the `await` (e.g., with `about:blank` and updating the location), or accepting this trade-off since Electron's default renderer doesn't enforce popup blocking.</comment>
<file context>
@@ -410,19 +409,66 @@ function CompactMetaRow({
+ const result = await pear.broker.mintObserverToken(projectId)
+ const url = observerUrlForToken(result.token)
+ setState({ status: 'ready', url })
+ window.open(url, '_blank', 'noreferrer')
+ } catch (err) {
+ setState({ status: 'error', message: err instanceof Error ? err.message : String(err) })
</file context>
There was a problem hiding this comment.
Investigated — I don't think this applies to this app, and the suggested fix wouldn't actually work here even if it did.
Every window.open() call in this renderer goes through mainWindow.webContents.setWindowOpenHandler in src/main/index.ts (~line 140), which unconditionally does { action: 'deny' } + shell.openExternal(details.url) — regardless of the calling context's gesture recency. So there's no real Electron popup window for a gesture-timing heuristic to block; the actual navigation happens via shell.openExternal in the main process.
More importantly: per Electron's documented behavior, when setWindowOpenHandler returns { action: 'deny' }, window.open() returns null in the renderer. That means the suggested "open about:blank synchronously, set .location after the await" pattern would get null back from the synchronous about:blank call here — there'd be no window handle to later navigate, so the fix would be a no-op (or throw on null.location) rather than a real mitigation. It would also fire shell.openExternal('about:blank') up front, which risks flashing a blank tab in the user's system browser for no benefit.
Given both of those, I'm accepting the trade-off (as the comment itself suggests) rather than applying a fix that wouldn't function correctly in this codebase's actual window-open path. Not changing this.
There was a problem hiding this comment.
You're right — the popup-blocker concern doesn't apply here. window.open() is denied in setWindowOpenHandler, so the renderer never gets a real window handle to preserve, and the about:blank workaround would be a no-op. I'll drop that comment for this PR.
| json: async () => ({ token: 'ot_live_abc', id: 'obs-1' }), | ||
| text: async () => '' | ||
| })) | ||
| vi.stubGlobal('fetch', fetchMock) |
There was a problem hiding this comment.
P2: Global fetch stubs in these tests rely on vi.unstubAllGlobals() at the end of the test body, but if an assertion fails before that line, the stub leaks into subsequent tests causing cascading failures. Consider adding vi.unstubAllGlobals() to the file-level afterEach (or wrapping in try...finally) to ensure cleanup always runs regardless of test outcome.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/main/broker.test.ts, line 845:
<comment>Global `fetch` stubs in these tests rely on `vi.unstubAllGlobals()` at the end of the test body, but if an assertion fails before that line, the stub leaks into subsequent tests causing cascading failures. Consider adding `vi.unstubAllGlobals()` to the file-level `afterEach` (or wrapping in `try...finally`) to ensure cleanup always runs regardless of test outcome.</comment>
<file context>
@@ -832,6 +832,81 @@ describe('BrokerManager local + cloud coexistence', () => {
+ json: async () => ({ token: 'ot_live_abc', id: 'obs-1' }),
+ text: async () => ''
+ }))
+ vi.stubGlobal('fetch', fetchMock)
+
+ const result = await manager.mintObserverToken(PROJECT_ID)
</file context>
There was a problem hiding this comment.
This is a stale re-flag, not a newly-introduced instance — verified against the current file. This exact concern was already fixed in commit 4ca7bd6 (pushed before this cubic review ran): vi.unstubAllGlobals() was moved out of each test body into the enclosing describe('BrokerManager local + cloud coexistence', ...) block's existing afterEach (src/main/broker.test.ts line ~643), which runs unconditionally regardless of test outcome, and the redundant per-test calls were removed. The diff hunk this comment quotes only shows the vi.stubGlobal('fetch', fetchMock) call in isolation without visibility into that shared afterEach ~200 lines later, so it's flagging code that no longer has the described gap.
The two new tests added in d681de2 (coalescing + 404-message coverage) also stub fetch and rely on the same shared afterEach for cleanup — no per-test vi.unstubAllGlobals() was reintroduced. No change needed here.
There was a problem hiding this comment.
You're right — the parent comment is stale here. The shared afterEach in src/main/broker.test.ts already runs unconditionally and cleans up vi.stubGlobal('fetch', ...), so this diff hunk alone was misleading.
…essage - mintObserverToken: coalesce concurrent in-flight requests per project behind one promise (observerTokenMints map, same keyed-promise/finally shape as SpawnCoordinator.coalesce), so a double-click or two open BrokerDetailsPage views can't each race past the cache-miss check and mint a separate, unrevoked token before the first POST resolves. - On a 404 from /api/observer-token (the bundled broker dependency predating this route), surface a distinct "Observer links require a newer broker version — restart Pear after upgrading" error instead of a generic HTTP-error message. Deliberately does NOT fall back to the old workspace-key-based link on 404 — that would silently reintroduce the full-workspace-key leak this PR exists to fix. - Added coalescing + 404-message test coverage in broker.test.ts. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/renderer/src/components/broker/BrokerDetailsPage.tsx">
<violation number="1" location="src/renderer/src/components/broker/BrokerDetailsPage.tsx:441">
P2: The `window.open` after minting succeeds runs outside the original click handler's gesture context (after `await`), so it may be blocked by popup blockers in configurations where `nativeWindowOpen: true` or in test/web contexts. The 'ready' state fast-path on line 434 is fine since it's synchronous. Consider opening the window before the `await` (e.g., with `about:blank` and updating the location), or accepting this trade-off since Electron's default renderer doesn't enforce popup blocking.</violation>
</file>
<file name="src/main/broker.test.ts">
<violation number="1" location="src/main/broker.test.ts:845">
P2: Global `fetch` stubs in these tests rely on `vi.unstubAllGlobals()` at the end of the test body, but if an assertion fails before that line, the stub leaks into subsequent tests causing cascading failures. Consider adding `vi.unstubAllGlobals()` to the file-level `afterEach` (or wrapping in `try...finally`) to ensure cleanup always runs regardless of test outcome.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
observerTokenMints (the in-flight-mint coalescing map added for the prior double-click fix) wasn't cleared in dropSession() — only the resolved observerTokens cache was. If a session dropped mid-mint (e.g. joinWorkspace rejoining a different workspace) and a new session started for the same projectId, the restarted session's mintObserverToken call could reuse the old session's stale in-flight promise, then cache ITS result after the resolved cache had already been cleared — pointing the new session's observer link at the previous workspace's token. Fix, mirroring the existing eventStreamGeneration guard idiom used elsewhere in this file: - dropSession() now also deletes the project's observerTokenMints entry and bumps a new per-project generation counter (observerTokenGenerations). - mintObserverToken captures the generation in flight when a mint starts, and mintObserverTokenUncached only writes to observerTokens if that generation is still current when the fetch resolves — so a stale mint from a dropped session can still resolve for whoever was already awaiting it directly, but can no longer poison the cache for the new session. Added a regression test simulating the exact race (mint in flight -> session torn down and restarted -> stale fetch resolves late) and asserting the restarted session mints fresh and the cache isn't overwritten afterward. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Pulls in relay #1223 (POST /api/observer-token, what this PR's mintObserverToken actually calls) and #1224 (dashboard replay durability fixes), both released as 9.2.0. Without this bump the bundled broker binary doesn't register the new route and observer links 404 (the exact gap flagged on this PR's own review thread). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 participant
Summary
Two independent broker-pipeline fixes:
1. "Join as observer" no longer leaks the full workspace API key (security fix)
BrokerDetailsPage.tsx's observer link previously embedded the raw Relaycastworkspace API key (
rk_live_...) — the exact credential used forRELAY_API_KEYwhen spawning agents (full read/write/spawn access) — in ahttps://agentrelay.com/observer?key=...URL. Anyone given that "observer"link actually got full workspace control.
BrokerManager.mintObserverToken(projectId)insrc/main/broker.ts,which POSTs directly to the broker's own local
/api/observer-tokenendpoint (
X-API-Key: <broker's local key>), mirroring the existinggetClientApiKey/getClientBaseUrlpattern already used elsewhere in thisfile for direct broker HTTP calls (no new pattern introduced). Response
{ token, id }is cached in-memory perprojectIdfor the life of thatbroker session (cleared in
dropSession(), e.g. onjoinWorkspaceshutdown+restart), so repeatedly opening the UI doesn't mint fresh,
unrevoked tokens.
broker:mint-observer-token(main handler inipc-handlers.ts, preload insrc/preload/index.ts, typed insrc/shared/types/ipc.tsaspear.broker.mintObserverToken(projectId): Promise<ObserverTokenResult>), following the same end-to-end wiring asjoinWorkspace/workspaceKeyForProject.BrokerDetailsPage.tsx: replaced the synchronousobserverUrlForWorkspaceKeywith an async
ObserverLinkActioncomponent with minting/error UI states(spinner while minting, inline error + retry on failure), preserving the
existing compact copy/open-link UX. The resulting link is
https://agentrelay.com/observer?key=<minted ot_live_ token>— same URLshape as before, scoped token instead of the raw workspace key.
Depends on a parallel
relayrepo PR adding the broker-side/api/observer-tokenendpoint, and a parallelrelaycastrepo PR making theobserver-dashboard web app accept
ot_live_tokens on thekeyparam. ThisPR alone does not fully close the loop until both land — the IPC/UI plumbing
is in place and ready to work against the agreed contract.
2. Unhandled
replay_gapsilently dropped missed channel messagesThe broker's local dashboard WebSocket (which
relay_inboundchat messagesflow over, among everything else) can send
{"kind": "replay_gap", "requestedSinceSeq", "oldestAvailable", "seq"}when its replay buffer can'tcover a reconnect (e.g. a burst of
worker_streamchunks evicted a pendingchat message before replay). Nothing in this codebase handled that
kindanywhere, so a dropped message just vanished with no trace.
src/shared/schemas/broker-events.ts: addedreplay_gapto the knownevent schema (previously fell through the generic "unknown kind,
forward-compat" path).
src/main/broker.ts: added an explicit ingress branch that logs the gap(still forwarded to the renderer like any other event).
src/renderer/src/stores/agent-store.ts:handleBrokerEventnow bumps alastReplayGapAttimestamp onkind === 'replay_gap'.src/renderer/src/hooks/use-message-reconciliation.ts: watcheslastReplayGapAt(mirroring the existinglastHumanMessageSentAtwiring)and calls
reconciler.schedule('replay-gap')— reusing the existingdebounced
reconcileMessagesresync pipeline (createMessageReconciler/reconcileChatMessages) rather than building a parallel mechanism. Thereconciler's single shared debounce timer already collapses a burst of
gaps into one resync, so this can't spam the broker even if gaps fire
repeatedly in a short window.
Test plan
npm run typecheck— passesnpm test— 123/123 passing (baseline unchanged,src/main/__tests__node:test suite)
npx vitest run— 494/494 passing across 34 files, including new coverage:src/main/broker.test.ts:mintObserverTokenmint/cache/error/re-mint-after-shutdown behavior;
replay_gapclassification + ingressforwarding/logging
src/renderer/src/stores/agent-store.test.ts:handleBrokerEventbumps
lastReplayGapAtonreplay_gapwithout touching agents/messagessrc/renderer/src/hooks/use-message-reconciliation.test.ts:scheduleReplayGapReconciliationbehavior + hook wiring (source-inspectiontest, matching the existing
lastHumanMessageSentAtpattern)npm run dev— per instructions, there's a live dev sessionrunning against the non-worktree checkout; verification relied on
typecheck + unit tests only.
Deviations from spec
None of substance. The
/api/observer-tokenrequest/response contract isimplemented exactly as given (
POSTwithX-API-Key, optionalworkspaceId/workspaceAlias/namebody,{ token, id, workspaceId, workspaceAlias }response — Pear only needstoken/id). Caching choice(mint-once-per-project, in-memory, no revocation UI) matches the suggested
default in the task description.
Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com