From f558bbd9158227bd44d2a7682950beed0568eae0 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Thu, 20 Aug 2026 23:25:48 -0700 Subject: [PATCH 1/3] Add test for out-of-band GitHub credential settling a template-owned room MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Proves a credential completed outside the connect-github card's own submit (e.g. the Plugins page) settles a room whose card registered under the code-review template's own `template/pendingConnections` key — fails against today's settleConnectedService, which only ever looks at `connections/pending`. --- packages/chat/test/connect-pending.test.ts | 51 ++++++++++++++++++++++ 1 file changed, 51 insertions(+) diff --git a/packages/chat/test/connect-pending.test.ts b/packages/chat/test/connect-pending.test.ts index 2bb8df048..c9e71059e 100644 --- a/packages/chat/test/connect-pending.test.ts +++ b/packages/chat/test/connect-pending.test.ts @@ -35,6 +35,27 @@ async function seedWorkbench( }); } +async function seedTemplateWorkbench( + store: ReturnType, + workbenchId: string, + templatePending: readonly string[], +) { + await store.createWorkbenchSettings({ + tenantId: TENANT.id, + workbenchId, + settings: { + "chat/kind": "workbench", + "chat/participants": [ + { address: HUMAN_ADDRESS, handle: "owner" }, + { address: AGENT_ADDRESS, handle: "myra" }, + ], + "template/id": "code-review", + "template/pendingConnections": templatePending, + }, + updatedBy: "prn_owner", + }); +} + function buildDeps() { const store = createInMemoryChatStore(); const roomMessages = createInMemoryRoomMessageStore(); @@ -110,6 +131,36 @@ test("matches a pending mcp-prefixed entry when the preset connects under its ba expect(settled?.settings["connections/pending"]).toEqual([]); }); +test("settles a room whose GitHub card is pending under the code-review template's own key — a credential created out of band (not through that card's own submit) still reaches it", async () => { + const { store, roomMessages, published, deps } = buildDeps(); + await seedTemplateWorkbench(store, "chan_template", ["github"]); + + await settleConnectedService(deps, { + tenantId: TENANT.id, + principalId: "prn_owner", + connectorId: "github", + displayName: "GitHub", + }); + + const settled = await store.getWorkbenchSettings(TENANT.id, "chan_template"); + expect(settled?.settings["template/pendingConnections"]).toEqual([]); + expect(settled?.settings["template/id"]).toBe("code-review"); + expect( + published.some( + (entry) => + entry.workbenchId === "chan_template" && + entry.event.type === "chat.settings", + ), + ).toBe(true); + + const listed = await roomMessages.listMessages({ + tenantId: TENANT.id, + workbenchId: "chan_template", + }); + expect(listed.items).toHaveLength(1); + expect(JSON.stringify(listed.items[0]?.parts)).toContain("GitHub"); +}); + test("a connector no room is waiting on settles nothing", async () => { const { store, roomMessages, published, deps } = buildDeps(); await seedWorkbench(store, "chan_1", ["exa"]); From 7e54e0adf2f72e19e169c74f5b7df67f4f21c952 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Thu, 20 Aug 2026 23:25:54 -0700 Subject: [PATCH 2/3] Settle the code-review template's own pending-connections key too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CL-6463: a credential connected anywhere other than the in-room GitHub card's own submit (the Plugins page, another tab) never reached that card, because settleConnectedService only ever cleared the generic connect-service key (connections/pending) — the code-review template's GitHub card registers under a second, template-owned key (template/pendingConnections) that this settle path never touched. Rather than stand up a second settle function for that one key, this folds it into the same settleConnectedService call: a connector becoming connected is one event, and every room waiting on it settles through one mechanism, not two parallel key conventions. --- packages/chat/src/connect-pending.ts | 55 ++++++++++++++++++++++++---- 1 file changed, 47 insertions(+), 8 deletions(-) diff --git a/packages/chat/src/connect-pending.ts b/packages/chat/src/connect-pending.ts index 2ebb5bbaf..b49b0cb69 100644 --- a/packages/chat/src/connect-pending.ts +++ b/packages/chat/src/connect-pending.ts @@ -7,6 +7,15 @@ // a message under the connecting person's own address — which routes to // the room's host agent through the ordinary message path, so the agent // resumes the task it parked without any new trigger machinery. +// +// CL-6463: the code-review template's own GitHub connect card registers +// under a second, template-owned key (`@corbits/workflow-catalog`'s +// `template/pendingConnections`) instead of `connections/pending` — a +// credential completed anywhere other than that card's own submit (the +// Plugins page, another tab) never reached it. Rather than stand up a +// second settle path for that one key, this module settles both: a +// connector becoming connected is one event, and every room's settling +// belongs to one mechanism, not two parallel key conventions. import { type } from "arktype"; import { @@ -19,16 +28,32 @@ import { ConnectServiceBlockData } from "./blocks"; export const CONNECTIONS_PENDING_KEY = "connections/pending"; +/** The code-review template's own pending-connections key + * (`@corbits/workflow-catalog`'s `templateSettingsPatch`/ + * `templateReposSettingsPatch`) — a room minted from that template + * tracks its GitHub card's pending state here instead of under + * `CONNECTIONS_PENDING_KEY`. `settleConnectedService` knows this one + * literal key so a credential settling still reaches that card, without + * standing up a second, template-scoped settle function. */ +const TEMPLATE_PENDING_CONNECTIONS_KEY = "template/pendingConnections"; + const PendingConnections = type("string[]"); -export function pendingConnectionsOf( +function pendingConnectionsAt( settings: Record, + key: string, ): readonly string[] { - const parsed = PendingConnections(settings[CONNECTIONS_PENDING_KEY]); + const parsed = PendingConnections(settings[key]); if (parsed instanceof type.errors) return []; return parsed; } +export function pendingConnectionsOf( + settings: Record, +): readonly string[] { + return pendingConnectionsAt(settings, CONNECTIONS_PENDING_KEY); +} + /** Connector ids named by `connect-service` block parts in a message — * parsed through the block's own schema so a malformed block registers * nothing. */ @@ -85,18 +110,32 @@ export async function settleConnectedService( ): Promise { const rows = await deps.store.listWorkbenchSettings(input.tenantId); const connected = bareConnectorId(input.connectorId); + const isSettled = (entry: string) => bareConnectorId(entry) === connected; for (const row of rows) { const pending = pendingConnectionsOf(row.settings); - if (!pending.some((entry) => bareConnectorId(entry) === connected)) { - continue; - } - const remaining = pending.filter( - (entry) => bareConnectorId(entry) !== connected, + const templatePending = pendingConnectionsAt( + row.settings, + TEMPLATE_PENDING_CONNECTIONS_KEY, ); + const matchedPending = pending.some(isSettled); + const matchedTemplatePending = templatePending.some(isSettled); + if (!matchedPending && !matchedTemplatePending) continue; + + const settingsPatch: Record = { ...row.settings }; + if (matchedPending) { + settingsPatch[CONNECTIONS_PENDING_KEY] = pending.filter( + (entry) => !isSettled(entry), + ); + } + if (matchedTemplatePending) { + settingsPatch[TEMPLATE_PENDING_CONNECTIONS_KEY] = templatePending.filter( + (entry) => !isSettled(entry), + ); + } const updated = await deps.store.updateWorkbenchSettings({ tenantId: input.tenantId, workbenchId: row.workbenchId, - settings: { ...row.settings, [CONNECTIONS_PENDING_KEY]: remaining }, + settings: settingsPatch, updatedBy: input.principalId, }); deps.publish(row.workbenchId, { From cac72621d0835ae104054dc45ecc5172d78cb42c Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Thu, 20 Aug 2026 23:26:05 -0700 Subject: [PATCH 3/3] Delete the never-wired connect-github settings-stream fold MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CL-6463: applyConnectGithubSettingsEvent (connect-github-stream.ts) had zero call sites outside its own test — createChatConnectGithubActions (the only ConnectGithubActions implementation) never wired it in; subscribeConnectState only ever fans out after its own actions run. With settleConnectedService now the one place that settles a connector for every waiting room (previous commit), this fold has no role left to wire it into, so it's dead code rather than a second mechanism worth keeping alive. Updates the doc comments across chat-ui/workflow-catalog that described it as already wired. --- .../src/blocks/connect-github-actions.ts | 16 ++++--- .../blocks/connect-github-block-container.tsx | 16 +++---- .../src/blocks/connect-github-stream.test.ts | 48 ------------------- .../src/blocks/connect-github-stream.ts | 41 ---------------- packages/chat-ui/src/index.ts | 1 - .../connect-github-block-container.test.tsx | 6 +-- .../chat-ui/test/connect-github-flow.test.tsx | 37 +++++--------- .../src/connect-github-routes.ts | 5 +- .../src/connect-github-setup.ts | 6 +-- packages/workflow-catalog/src/settings.ts | 7 +-- 10 files changed, 43 insertions(+), 140 deletions(-) delete mode 100644 packages/chat-ui/src/blocks/connect-github-stream.test.ts delete mode 100644 packages/chat-ui/src/blocks/connect-github-stream.ts diff --git a/packages/chat-ui/src/blocks/connect-github-actions.ts b/packages/chat-ui/src/blocks/connect-github-actions.ts index f124bfee4..5c10ff775 100644 --- a/packages/chat-ui/src/blocks/connect-github-actions.ts +++ b/packages/chat-ui/src/blocks/connect-github-actions.ts @@ -29,12 +29,16 @@ export type ConnectGithubActions = { * message's own `ConnectGithubBlockData`. */ readonly getConnectState: (messageId: string) => Promise; /** - * Folds this room's live stream straight into the card's state — - * never a second `getConnectState` call. The host wires this to the - * workbench's existing `chat.settings` SSE event (folded through - * `./connect-github-stream.ts`'s `applyConnectGithubSettingsEvent`), - * the same event `templateReposSettingsPatch` writes onto once a - * person starts reviewing repos. Returns an unsubscribe. + * Registers for this card's state updates — the host fans an update + * out to every subscriber after its own actions (`submitAccessToken`, + * `startReviewing`, `skip`) change something, re-reading + * `getConnectState`. A credential completed elsewhere (the Plugins + * page, another tab) settles this connector's entry on + * `@corbits/workflow-catalog`'s `template/pendingConnections` + * (CL-6463's `settleConnectedService`) so the *next* fresh + * `getConnectState` — e.g. on this card's next mount — already reads + * connected, since that read resolves against the real credential, + * never this setting. Returns an unsubscribe. */ readonly subscribeConnectState: ( messageId: string, diff --git a/packages/chat-ui/src/blocks/connect-github-block-container.tsx b/packages/chat-ui/src/blocks/connect-github-block-container.tsx index 6a1547538..86eb74479 100644 --- a/packages/chat-ui/src/blocks/connect-github-block-container.tsx +++ b/packages/chat-ui/src/blocks/connect-github-block-container.tsx @@ -6,14 +6,14 @@ // framing every other block's "no port, no feature" fallback uses. // // CL-6463: a card's own successful PAT submit is the one change this -// container never waits on a fold for. `subscribeConnectState` folds -// whatever a host chooses to publish, and the room's `chat.settings` -// event (the only thing `connect-github-stream.ts` can fold) is written -// by the later, unrelated repo-review PATCH — never by the credential -// save itself. So `submitAccessToken` gets its own explicit -// `getConnectState` refetch here, run once as the direct consequence of -// that one submit — not a poll, and not a second source of truth -// alongside the fold; the fold keeps handling every other update. +// container never waits on the host to fan out on its own — a credential +// saved through *this* card's field gets its own explicit `getConnectState` +// refetch below, run once as the direct consequence of that one submit +// (not a poll). A credential saved anywhere else (the Plugins page, +// another tab) settles through `packages/chat/src/connect-pending.ts`'s +// `settleConnectedService`, which clears this room's own +// `template/pendingConnections` entry — so this card's next mount already +// reads connected without needing a push while it sits open. import { useCallback, useEffect, useRef, useState } from "react"; import type { ConnectGithubBlockData } from "@corbits/chat/blocks"; diff --git a/packages/chat-ui/src/blocks/connect-github-stream.test.ts b/packages/chat-ui/src/blocks/connect-github-stream.test.ts deleted file mode 100644 index 769eae249..000000000 --- a/packages/chat-ui/src/blocks/connect-github-stream.test.ts +++ /dev/null @@ -1,48 +0,0 @@ -import { expect, test } from "bun:test"; - -import type { ChatSettingsEventData } from "@corbits/chat/stream-events"; - -import type { ConnectGithubRepo } from "./connect-github-actions"; -import { applyConnectGithubSettingsEvent } from "./connect-github-stream"; - -const REPOS: readonly ConnectGithubRepo[] = [ - { id: "1", name: "acme/widgets", openPullRequestCount: 0 }, -]; - -test("folds a settled settings event into the connected state", () => { - const event: ChatSettingsEventData = { - updatedBy: "prn_owner", - settings: { - "template/pendingConnections": [], - "template/selectedRepos": ["1"], - }, - }; - expect( - applyConnectGithubSettingsEvent(event, "github", "octocat", REPOS), - ).toEqual({ - kind: "connected", - orgName: "octocat", - repos: REPOS, - selectedRepoIds: ["1"], - }); -}); - -test("folds an event whose connector is still pending into the disconnected state", () => { - const event: ChatSettingsEventData = { - updatedBy: "prn_owner", - settings: { "template/pendingConnections": ["github"] }, - }; - expect( - applyConnectGithubSettingsEvent(event, "github", "octocat", REPOS), - ).toEqual({ kind: "disconnected" }); -}); - -test("ignores a settings event carrying no template/* keys at all", () => { - const event: ChatSettingsEventData = { - updatedBy: "prn_owner", - settings: { "chat/theme": "dark" }, - }; - expect( - applyConnectGithubSettingsEvent(event, "github", "octocat", REPOS), - ).toBeUndefined(); -}); diff --git a/packages/chat-ui/src/blocks/connect-github-stream.ts b/packages/chat-ui/src/blocks/connect-github-stream.ts deleted file mode 100644 index 5b35c4e79..000000000 --- a/packages/chat-ui/src/blocks/connect-github-stream.ts +++ /dev/null @@ -1,41 +0,0 @@ -// Folds a room's existing `chat.settings` stream event straight into a -// connect-github card's live state (CL-6345) — no new event type, no -// refetch. `@corbits/workflow-catalog`'s `templateReposSettingsPatch` -// writes `template/pendingConnections` (with `"github"` removed) and -// `template/selectedRepos` in one PATCH once a person starts reviewing -// repos; that PATCH's own route already publishes `chat.settings` with -// the full post-change settings object (`packages/chat/src/routes.ts`). -// A host wires this function as the fold behind `ConnectGithubActions`' -// `subscribeConnectState`. -import type { ChatSettingsEventData } from "@corbits/chat/stream-events"; - -import type { - ConnectGithubQuery, - ConnectGithubRepo, -} from "./connect-github-actions"; - -/** - * Reads `event.settings` for this connector's own settled state. - * Returns `undefined` when the event carries no `template/*` keys at - * all — a settings change unrelated to this card, which a subscriber - * should ignore rather than fold into a stale-looking update. - */ -export function applyConnectGithubSettingsEvent( - event: ChatSettingsEventData, - connectorId: string, - orgName: string, - repos: readonly ConnectGithubRepo[], -): ConnectGithubQuery | undefined { - const pending = event.settings["template/pendingConnections"]; - if (!Array.isArray(pending)) return undefined; - if (pending.includes(connectorId)) { - return { kind: "disconnected" }; - } - const selected = event.settings["template/selectedRepos"]; - return { - kind: "connected", - orgName, - repos, - selectedRepoIds: Array.isArray(selected) ? selected.map(String) : [], - }; -} diff --git a/packages/chat-ui/src/index.ts b/packages/chat-ui/src/index.ts index 99d90d3c3..c382d758c 100644 --- a/packages/chat-ui/src/index.ts +++ b/packages/chat-ui/src/index.ts @@ -118,7 +118,6 @@ export type { ConnectServiceResult, ConnectAffordance, } from "./blocks/connect-service-actions"; -export { applyConnectGithubSettingsEvent } from "./blocks/connect-github-stream"; export { TextPart, diff --git a/packages/chat-ui/test/connect-github-block-container.test.tsx b/packages/chat-ui/test/connect-github-block-container.test.tsx index 1a75e261c..ae0e9b98c 100644 --- a/packages/chat-ui/test/connect-github-block-container.test.tsx +++ b/packages/chat-ui/test/connect-github-block-container.test.tsx @@ -1,8 +1,8 @@ // CL-6463: a successful PAT submit must flip the connect-github card to // connected on its own — never leaning on a host that happens to fan the -// change out through `subscribeConnectState` (real hosts vary, and -// `chat.settings` never carries the credential-save path at all; see -// `connect-github-stream.ts`'s own header). These fakes deliberately never +// change out through `subscribeConnectState` (real hosts vary: the real +// `createChatConnectGithubActions` only fans out after its own actions, +// never off a live `chat.settings` push). These fakes deliberately never // call the subscriber from `submitAccessToken`, so a pass here proves the // container drove its own state from the submit's own result — not from a // side channel a differently-wired host might forget. diff --git a/packages/chat-ui/test/connect-github-flow.test.tsx b/packages/chat-ui/test/connect-github-flow.test.tsx index 6d6c7a8f4..14175c4a8 100644 --- a/packages/chat-ui/test/connect-github-flow.test.tsx +++ b/packages/chat-ui/test/connect-github-flow.test.tsx @@ -7,16 +7,14 @@ // 1. `@corbits/workflow-catalog`'s real `startReviewingRepos` mints // one grant and one webhook trigger per selected repo, and records // the selection. -// 2. The card settles into its connected state purely by folding a -// `chat.settings` stream event through the real -// `applyConnectGithubSettingsEvent` — never a second -// `getConnectState` fetch. +// 2. The card settles into its connected state by the host fanning an +// update out to `subscribeConnectState`'s listener — the same +// channel a real host fans out on after any of its own actions. import { afterEach, describe, expect, test } from "bun:test"; import { act } from "react"; import { createRoot } from "react-dom/client"; import type { Root } from "react-dom/client"; -import type { ChatSettingsEventData } from "@corbits/chat/stream-events"; import { startReviewingRepos, type ConnectGithubSetupPorts, @@ -28,7 +26,6 @@ import type { ConnectGithubQuery, ConnectGithubRepo, } from "../src/blocks/connect-github-actions"; -import { applyConnectGithubSettingsEvent } from "../src/blocks/connect-github-stream"; import { WorkbenchTimeline } from "../src/timeline"; const REPOS: readonly ConnectGithubRepo[] = [ @@ -60,10 +57,8 @@ function messageWithConnectGithubBlock(): MessageItem[] { /** The whole flow's fakes, wired the way a real host would wire them: * `ConnectGithubActions.startReviewing` calls the real * `startReviewingRepos` against fake grant/trigger/settings ports, then - * simulates the room's settings-PATCH route publishing `chat.settings` - * (the real plumbing every `template/*` write already rides — see - * `packages/chat/src/routes.ts`), folded through the real - * `applyConnectGithubSettingsEvent`. */ + * fans the settled state out to `subscribeConnectState`'s listener — + * the same thing a real host does after any of its own actions. */ function buildHarness() { const grantedRepos: string[] = []; const createdTriggerRepos: string[] = []; @@ -123,20 +118,12 @@ function buildHarness() { }, async startReviewing(repoIds) { const result = await startReviewingRepos(repoIds, REPOS, setupPorts); - const settingsEvent: ChatSettingsEventData = { - updatedBy: "prn_owner", - settings: { - "template/pendingConnections": [], - "template/selectedRepos": repoIds, - }, - }; - const folded = applyConnectGithubSettingsEvent( - settingsEvent, - "github", - "octocat", - REPOS, - ); - if (folded !== undefined) subscriber?.(folded); + subscriber?.({ + kind: "connected", + orgName: "octocat", + repos: REPOS, + selectedRepoIds: repoIds, + }); return { startedTriggerCount: result.createdTriggerIds.length }; }, async skip() {}, @@ -193,7 +180,7 @@ describe("connect-github round trip (CL-6345)", () => { expect(el.querySelector(".chat-block-connect-repo-row")).toBeNull(); }); - test("connect -> list repos -> pick three -> start reviewing mints a grant and a webhook trigger per repo, and settles into the connected state via the stream, never a second fetch", async () => { + test("connect -> list repos -> pick three -> start reviewing mints a grant and a webhook trigger per repo, and settles into the connected state via the host's fan-out, never a second fetch", async () => { const harness = buildHarness(); const el = await mount(harness.actions); diff --git a/packages/workflow-catalog/src/connect-github-routes.ts b/packages/workflow-catalog/src/connect-github-routes.ts index ff7ab64a3..3eb02a59c 100644 --- a/packages/workflow-catalog/src/connect-github-routes.ts +++ b/packages/workflow-catalog/src/connect-github-routes.ts @@ -101,8 +101,9 @@ export type ConnectGithubRoutesDeps = { ): Promise; /** Applies the settings PATCH `./settings.ts`'s * `templateReposSettingsPatch` builds and publishes the room's - * `chat.settings` stream event — the same event `applyConnectGithubSettingsEvent` - * (`@corbits/chat-ui`) folds the connect card's live state from. */ + * `chat.settings` stream event — the connect card refetches its own + * state as the direct consequence of the `start-reviewing` call that + * triggers this patch, never a fold off the stream event itself. */ persistSelectedRepos( tenantId: string, workbenchId: string, diff --git a/packages/workflow-catalog/src/connect-github-setup.ts b/packages/workflow-catalog/src/connect-github-setup.ts index e86705f27..b32c4d9e8 100644 --- a/packages/workflow-catalog/src/connect-github-setup.ts +++ b/packages/workflow-catalog/src/connect-github-setup.ts @@ -31,9 +31,9 @@ export interface ConnectGithubSetupPorts { * Records which repos this room is reviewing — the `template/*` * settings namespace's `selectedRepos` key (`./settings.ts`'s * `templateReposSettingsPatch`). A host binds this to the room's - * existing settings PATCH route, whose own `chat.settings` stream - * event is what a connect-github card folds its connected state from - * — see `@corbits/chat-ui`'s `applyConnectGithubSettingsEvent`. + * existing settings PATCH route; the connect-github card refetches + * its own state as the direct consequence of the call that triggers + * this patch, never a fold off the resulting stream event. */ persistSelectedRepos(repoIds: readonly string[]): Promise; } diff --git a/packages/workflow-catalog/src/settings.ts b/packages/workflow-catalog/src/settings.ts index 28f2c388b..e36c5c0d0 100644 --- a/packages/workflow-catalog/src/settings.ts +++ b/packages/workflow-catalog/src/settings.ts @@ -40,9 +40,10 @@ export function templateSettingsPatch( * `"github"` removed (this template needs nothing else, so that leaves * it empty) and `selectedRepos` naming exactly what got a live webhook * trigger. This patch rides the same `chat/*` settings PATCH route - * every other `template/*` write does, so the room's existing - * `chat.settings` stream event is what a connect-github card folds its - * connected state from — no bespoke event, no refetch. + * every other `template/*` write does; the card itself refetches its + * state as the direct consequence of the `start-reviewing` call that + * triggers this patch, same as it does for its own PAT submit + * (`ConnectGithubBlockContainer`) — never a fold off this stream event. */ export const TemplateReposSettingsPatch = type({ "template/pendingConnections": "string[]",