CL-6463: Settle the code-review template's GitHub connect card on out-of-band credentials - #207
Merged
Merged
Conversation
…room 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`.
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.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The owner's symptom: connecting GitHub on the Plugins page showed connected there, but the in-room GitHub connect card never advanced.
Root cause (confirmed by path trace, not re-diagnosed here):
settleConnectedService(packages/chat/src/connect-pending.ts) — the one function every connector-completion route (POST /connections/:connectorId/complete, OAuth callback, MCP preset) calls through the hub'sonConnectedhook — only ever cleared the genericconnections/pendingkey. The code-review template's own GitHub card registers under a second, template-owned key,template/pendingConnections(@corbits/workflow-catalog), whichsettleConnectedServicenever touched. A second gap compounded it: the client-side fold meant to read that key off a livechat.settingsevent (applyConnectGithubSettingsEvent,connect-github-stream.ts) had zero call sites outside its own test — nothing in production ever wired it in.Fix
Picked option (a): unified both keys into the one
settleConnectedServicemechanism rather than standing up a second settle path — "a connector became connected" is one event; it should have one place that tells every room waiting on it, not two parallel key conventions.packages/chat/src/connect-pending.ts:settleConnectedServicenow checks bothconnections/pendingandtemplate/pendingConnectionsper room, clearing whichever matched, in the same settings patch + singlechat.settingspublish + single resume message.packages/chat-ui/src/blocks/connect-github-stream.ts(and its test) —applyConnectGithubSettingsEventwas dead code with no production caller; kept it around would have been the exact "two parallel mechanisms" problem this fix is supposed to remove. Updated the doc comments inchat-ui/workflow-catalogthat described it as already wired.What this does and doesn't fix
GET .../github/state) resolves off the real stored credential, never offtemplate/pendingConnections— so this setting was never what gated the card's own connected/disconnected render. What it does gate is the room's agent resuming the parked code-review setup task, via the same resume-message mechanismconnections/pendingalready used — that path was fully broken for GitHub before this fix (a credential connected out of band never posted the resume message into a template room at all).connections/pendingnortemplate/pendingConnectionstoday has any live client-side subscriber — bothConnectGithubActions/ConnectServiceActionsimplementations inapps/webonly fan an update out to a mounted card after their own actions (submit/start-reviewing/skip), never off a realchat.settingsSSE push (confirmed:chat-workspace.tsx's stream switch has nochat.settingscase at all). So a card sitting mounted in a room while a credential is connected on a different tab/page will still only pick up the new state on its next remount, not instantly. That live-push gap is real and pre-existing for both connect flows, not something this PR introduces or fixes — flagging it rather than silently leaving it unverified.Test plan
packages/chat/test/connect-pending.test.tsseeds a room withtemplate/pendingConnections: ["github"](noconnections/pendingat all) and assertssettleConnectedServiceclears it, publisheschat.settings, and posts the resume message — confirmed failing against pre-fix code, passing after.bun testclean inpackages/chat(127 pass),packages/chat-ui(649 pass),packages/workflow-catalog(87 pass).tsc --noEmitclean in all three touched packages.bun run lintclean from repo root (0 errors; pre-existing warnings unrelated to this change).DO NOT MERGE — needs a peer review pass, including the live-proof gap called out above.