fix(console): stop offering the inert native OAuth catalog (#822) - #828
Conversation
…sai#822) The Connections page listed the native OAuth catalog as something you can connect, and the route it offered was not broken — which is what made it worth removing. `POST …/connections/{provider}/start` completes a real handshake against a provider application the operator registered, and the callback stores `oauth/{provider}` — read by no agent tool anywhere under `src/harness/` (tinyhumansai#396). So a self-hoster could do everything the page asked, watch the tile go green, and give their agents nothing. tinyhumansai#599 removed the Connect buttons that failed; this is the one that succeeded and bought nothing. - `connectRoute` loses its `native` arm entirely — the variant is gone from `ConnectRoute`, not merely deprioritised, so no tier and reach combination can reach it. A `static` host takes the Composio route where it has one and reports `unavailable` where it does not. - `buildGridProviders` stops appending `CONNECTION_PROVIDERS` tiles the backend catalog does not carry. It keeps its metadata role; a provider now appears because the host offers it, not because the console has a logo. - The tail it appends instead is what the host reports as **connected**, so retracting the offer cannot hide a credential the company already stored: the tile, its `via: ["native"]` and its Disconnect all survive. `providerId` gained a host-spelling fallback so that Disconnect can still name a connected provider the console has no tile for. - An honest empty state for a host whose catalog really is empty, since the native fallback used to paper over it. The host routes are untouched: this is the console declining to offer a path, not the path being removed. Settling tinyhumansai#396 makes the offer honest again and reinstating it is one arm in `connectRoute`. Verified against a live host with the hatch genuinely open (a local stand-in provider behind `OPENCOMPANY_OAUTH_SLACK_AUTHORIZE_URL`/`_TOKEN_URL`): the pre-fix bundle offered Connect on Slack, completed a real handshake and stored the credential; the post-fix bundle over the same data dir drops the five console-only tiles, offers no Connect, and still shows and releases that connection. `npm run typecheck`, `typecheck:e2e`, 603 unit tests and 127 Playwright specs pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 53 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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 selected for processing (3)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughThe console no longer offers native OAuth connections. Composio is the only connectable route. Existing native connections remain visible and disconnectable. Provider tiles now use the backend catalog and connected host data. ChangesNative route removal
Estimated code review effort: 3 (Moderate) | ~25 minutes Mergeability Score: ⚪ Minimal · up to The PR stops offering an OAuth connection path that provides no usable agent capability while preserving visibility and disconnection for existing native credentials. No actionable merge-blocking risk remains after normal checks and review. Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
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 |
oxoxDev
left a comment
oxoxDev
left a comment
There was a problem hiding this comment.
The framing is what makes this worth doing:
The route it offered was not broken, and that is what made it worth removing … complete a real OAuth handshake, see the tile go green, and have given their agents nothing.
#599 removed the buttons that failed; this removes the one that succeeded and bought nothing. A working control that confers no capability is harder to notice and worse to hit than a broken one, because nothing ever tells the operator.
Deleting { kind: "native" } from ConnectRoute entirely rather than deprioritising it is the right move — no combination of tier and reach can reach an arm that does not exist, which is a stronger property than an ordering that a later edit can reshuffle. Same instinct as making an unscoped fetch grant unrepresentable in #739.
Naming why the old precedence existed, and what it missed, is the part I would keep:
It preserved their configuration by handing them a green tile and no capability.
That is a fair correction of a decision that was defensible on its own terms, and stating both halves means the next person does not restore it from the same reasoning.
Two things you handled that the removal could easily have broken:
- A provider connected natively still says so, with
via: ["native"]and Disconnect intact — so existing native connections are not orphaned by the catalog losing its tile-source role. providerId's middle fallback — local tile, then the host's own spelling, then the slug — soDELETE …/connections/{provider}can still name a connected provider the console has no tile for, which is newly possible now that its row is the only reason it has a tile at all. That is the kind of second-order breakage a removal usually ships with.
The honest empty state matters too: without the native fallback papering over an empty catalog, "No provider in All." had to stop reading as a broken filter.
0 major. 1 question. Approving.
Question — the trap is gone from the console, not from the host
POST …/connections/{provider}/start is still registered (src/server/ops/connections.rs:69). So the handshake that "succeeded and bought nothing" is still reachable — by a script, an older console, a bookmarked URL, or anyone reading docs/modules/server/README.md's route table. It still stores oauth/{provider}, and nothing under src/harness/ still reads it.
Your scope is deliberate and stated — "all of it in frontend/src/lib/, as the issue proposes" — so I am not asking you to widen it here. But the console was the only thing that made this discoverable, and removing it means the route is now a live path with no surface warning about it. Worth either a deprecation note at the route itself, or an issue to retire it alongside whatever resolves #396, so the decision is recorded where the next person will be standing.
Related, and narrower than when I raised it on #819: with the console no longer able to create native secrets, the residual set is bounded and shrinking rather than growing. That is a real improvement to the question I asked there — a Composio disconnect still leaves an oauth/{provider} secret behind, but after this, fewer of them can exist to be left.
Before merging: GitHub still reports mergeable: UNKNOWN and the branch is 12 commits behind. Worth a rebase before merge given how much of the connections surface has moved this week — #819 and #827 both land near this.
![tinysweeper[bot]](https://avatars.githubusercontent.com/in/4526585?s=60&v=4){kind=small}
There was a problem hiding this comment.
tinysweeper found nothing blocking. Approving.
$0.1039 · 86,310 in / 32,580 out · 57,347 cached (66%) · z-ai/glm-5.2, deepseek/deepseek-v4-pro
critique: $0.0729 · 29,787 in / 25,983 out · 12,906 cached (43%) · z-ai/glm-5.2, deepseek/deepseek-v4-pro
security: $0.0141 · 24,351 in / 3,354 out · 19,856 cached (82%) · z-ai/glm-5.2
tests: $0.0096 · 15,284 in / 2,135 out · 11,380 cached (74%) · z-ai/glm-5.2
description: $0.0072 · 16,888 in / 1,108 out · 13,205 cached (78%) · z-ai/glm-5.2
What this change touches12 files, +652 -226 across 8 components. It reaches 6 untouched components (60 graph nodes walked). 2 further components left out to keep the diagram readable. flowchart LR
n0["docs/modules/server<br/>2 files +149 -114"]:::changed
n1["frontend/src/lib<br/>2 files +168 -67<br/>1 finding"]:::flagged
n2["frontend/test/e2e<br/>1 file +163 -0"]:::changed
n3["frontend/test/unit<br/>2 files +120 -14"]:::changed
n4["frontend/src/views/connections<br/>2 files +30 -4"]:::changed
n5["frontend/src/views<br/>1 file +7 -26"]:::changed
n6["frontend/src/tour<br/>1 file +10 -0"]:::changed
n7["frontend/src/api<br/>1 file +5 -1"]:::changed
n8["frontend/src/views<br/>6 files reached"]:::impacted
n9["frontend/src/components/ui<br/>5 files reached"]:::impacted
n10["frontend/src/views/connections<br/>4 files reached"]:::impacted
n11["frontend/src/api<br/>2 files reached"]:::impacted
n12["frontend/src/components<br/>1 file reached"]:::impacted
n13["frontend/src/lib<br/>1 file reached"]:::impacted
n8 -->|18 refs| n11
n8 -->|15 refs| n13
n8 -->|14 refs| n9
n10 -->|13 refs| n9
n3 -->|11 refs| n6
n8 -->|6 refs| n10
n9 -->|6 refs| n13
n10 -->|4 refs| n8
n10 -->|4 refs| n11
n8 -->|3 refs| n6
n12 -->|2 refs| n8
n12 -->|2 refs| n11
n6 -->|1 ref| n12
n12 -->|1 ref| n6
n12 -->|1 ref| n13
classDef changed fill:#0d4429,stroke:#238636,color:#e6edf3
classDef impacted fill:#161b22,stroke:#6e7681,color:#c9d1d9
classDef flagged fill:#5a1e02,stroke:#d93f0b,color:#ffffff
classDef blocking fill:#67060c,stroke:#f85149,color:#ffffff
Green: changed. Grey: untouched, reached through an import or a call. Orange: has findings. Red: has a finding that blocks the merge.
Changed files
|
tinysweeper
Bot
added
the
priority: p3
tinyhumansai#819 (the connection detail panel) and tinyhumansai#821 (the MCP panel) landed while this was open, and both touch the surfaces this changes. Three conflicts, all in the same seam and none of them textual accidents: - `provider-grid.ts` — tinyhumansai#819 added the `composioAccounts` argument and its paragraph where this rewrote the tail's. Both kept; the connected tail and the `providerId` host-spelling fallback merged into the new body unchanged. - `ConnectionsView.tsx` — the import block. `armTourResume` comes back on the upstream side purely because it was adjacent to tinyhumansai#819's new imports; its caller is still gone, so it stays dropped. - `ProvidersSection.tsx` — the real one. tinyhumansai#819 added `openable` immediately below `connectable`, and `connectable` is the line this narrows to Composio. Kept both: a tile opens when Composio can describe it, and offers Connect only for the route that confers something. `ProviderDetail`'s "Composio and MCP only" note now says the other half is done — the native catalog is not listed either, so the case it declines to open mostly cannot arise from the grid. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Filed as #838, and keeping this PR frontend-only as its scope states. One correction that came out of writing it up, because it changes what #838 can wait for: #396 is already closed as completed (2026-08-08) — this PR is what made its offer honest. So there was nothing left to retire the route "alongside", and without its own issue the write path would have quietly outlived the issue it was closed under. #838 is that issue. Two things I verified against
#838 proposes the note at the route plus a decision on its fate, and argues Not touching the merge conflict on this branch — another session is resolving it. |
There was a problem hiding this comment.
tinysweeper found nothing blocking. Approving.
$0.0575 · 59,376 in / 17,647 out · 47,231 cached (80%) · z-ai/glm-5.2
critique: $0.0315 · 15,671 in / 11,686 out · 12,955 cached (83%) · z-ai/glm-5.2
security: $0.0075 · 10,818 in / 2,106 out · 9,253 cached (86%) · z-ai/glm-5.2
tests: $0.0106 · 15,647 in / 2,848 out · 12,920 cached (83%) · z-ai/glm-5.2
description: $0.0079 · 17,240 in / 1,007 out · 12,103 cached (70%) · z-ai/glm-5.2
| const slug = tile ? toolkitSlug(tile.toolkit) : toolkitSlug(state.provider); | ||
| if (!slug || offered.has(slug)) continue; | ||
| offered.add(slug); | ||
| connectedOnly.push({ |
There was a problem hiding this comment.
Connected tail leaves blank name when no local tile exists
In the new connected-only tail, a connected provider that has no local CONNECTION_PROVIDERS tile falls back to empty strings for name and description. This case is reachable by design — the whole point of the tail is that "a provider the host reports as connected gets a tile whether or not the catalog offers one," and such a provider may well have no local metadata. The result is a rendered tile with a blank name/description, even though the host's own spelling (state.provider) is available right there in the loop and would be a more useful fallback than "".
connectedOnly.push({
slug,
name: tile?.name ?? "",
description: tile?.description ?? "",
logo: null,
categories: [],
});[RULE] Add focused tests with every behavior change. ·
| </ul> | ||
|
|
||
| {visible.length === 0 && ( | ||
| {providers.length === 0 && ( |
There was a problem hiding this comment.
Empty-state message claims connected providers still appear in a branch that is,
The new empty-state branch is gated on providers.length === 0, yet the message it renders ends with "Anything this company has already connected still appears here." The author's own inline comment in this same block states that with the fallback removed "a host with no catalog has nothing to show." If providers is the list this grid renders from (which the surrounding visible.length === 0 && providers.length > 0 branch implies), then providers.length === 0 means nothing — including already-connected providers — appears in the grid, making that closing sentence false. Either the message should not claim connected providers still appear when the grid is provably empty, or the guard should be on a narrower "no catalog" condition rather than providers.length === 0.
[RULE] logic that does not do what the surrounding code implies it should ·
tinysweeper
Bot
added
priority: p2
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
…i#822) CI's `assert-md-line-cap.sh` went red: this branch's edits took `docs/modules/server/README.md` to 504 lines. The 500 cap is not arbitrary — CLAUDE.md's remedy is to split the topic into a focused file and link it from the folder's README, which is what `workflow-routes.md`, `pausing-workflows.md` and `authority.md` already are. Connections is the natural seam and the topic this branch edits: the hatch and why the console stopped offering it, the `credentialSource` tiers, the single status behind the one provider grid, and the two disconnect routes are one subject spanning three sections. Moved verbatim into `connections.md` with the headings promoted a level; the README keeps a pointer in the same shape the other three splits use, and lands at 374 lines — headroom rather than one line under the wire. Also corrects `ConnectionCredentialSource`'s `static` doc, which still read "Connect works". The handshake does; the console stopped offering it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
tinysweeper found nothing blocking. Approving.
$0.0714 · 59,446 in / 22,800 out · 44,532 cached (75%) · z-ai/glm-5.2
critique: $0.0440 · 16,297 in / 16,732 out · 12,897 cached (79%) · z-ai/glm-5.2
security: $0.0018 · 1,917 in / 516 out · 1,407 cached (73%) · z-ai/glm-5.2
tests: $0.0136 · 19,743 in / 3,204 out · 14,437 cached (73%) · z-ai/glm-5.2
description: $0.0121 · 21,489 in / 2,348 out · 15,791 cached (73%) · z-ai/glm-5.2
| offered.add(slug); | ||
| connectedOnly.push({ | ||
| slug, | ||
| name: tile?.name ?? "", |
There was a problem hiding this comment.
Assert the rendered name for a connected provider with no local tile
The connected-only tail constructs a tile with name: tile?.name ?? "" when no local CONNECTION_PROVIDERS entry matches the host row. A provider the console has no metadata for (the zoom-pro case the test exercises) therefore gets a tile whose name is the empty string. The test "lists a connected provider the console has no metadata for" asserts connected, canDisconnect, and providerId but never asserts on name, so a blank-rendering tile would pass the suite. If the UI does not fall back to the slug for display, this is a visible regression that no test catches.
[RULE] Add focused tests with every behavior change. ·
tinyhumansai#828 (tinyhumansai#822) merged, and tinyhumansai#678 landed a triage evaluator. One conflict, in `test/e2e/mock-brain.mjs`, and it is the interesting kind: both branches added a guard ahead of the directive arms for the same underlying reason. `servedDirectives` is per-process, so a `__MOCK_TOOL_CALL__` fires for the FIRST request carrying it and never again. tinyhumansai#678 found that a triage escalation is handed the operator's raw message, carries the directive, and burns it — leaving the agent's own turn with prose. tinyhumansai#820 found the mirror image: after an approval the directive is already spent, so the host's "Re-issue it now" instruction produced nothing and no approval-gated tool could run in that lane at all. Both arms kept, triage first. Everything after that point assumes an agent turn and a classification is not one. It could not currently reach the re-issue arm anyway — `findReissue` requires the instruction to be the LAST message, and a classification's last message is the operator's — but that is a property of one prompt, not a rule worth resting on. The module header said "the three arms" and listed three; two branches each added one without touching it, so it now enumerates all five and says why the order is load-bearing. Verified directly rather than by inference, since the ordering was the one judgement call here: driving the merged mock brain, a triage request carrying a directive answers `chatter` and consumes nothing, the agent's own turn with the same message still gets its tool call, and a re-issue after approval gets one from the spent directive. All three, in that order, against one process. Also finishes the `ProviderDetail` correction the previous merge started. Its header doc still carried the pre-tinyhumansai#820 claim in full — "`composio_execute` posts `{tool, arguments}` and no connection id, so nothing on this side selects an account" — while the rendered paragraph had already been fixed. The doc is what the next reader reasons from; it now says the decision is real and where it is made, and why the panel still marks nothing itself. cargo fmt/clippy clean, `cargo test --locked` 2344 passed, console typecheck x3 with 654 unit tests, build, design-tokens, md-cap and feature-lanes all green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2 participants
Summary
The Connections page still listed the native OAuth catalog as something you can connect. The route it offered was not broken, and that is what made it worth removing:
POST …/connections/{provider}/startcompletes a real handshake against a provider application the operator registered themselves, and the callback storesoauth/{provider}— which is read by no agent tool anywhere undersrc/harness/(#396; the only hit on this checkout is a fixture string inplanning/test.rs). So an operator on a self-hosted instance could click Connect on a native-only tile, complete a real OAuth handshake, see the tile go green, and have given their agents nothing.#599 removed the Connect buttons that failed. This is the one that succeeded and bought nothing. #819 held the other half of @oxoxDev's call on #404 — the native path got no detail view; the listing is what was left.
What changed
All of it in
frontend/src/lib/, as the issue proposes.connectRouteloses itsnativearm.{ kind: "native" }is gone fromConnectRouteentirely rather than merely deprioritised, so no combination of tier and reach can reach it. A host reportingstatictakes the Composio route where it has one, and reportsunavailable— which the tile already renders without an action — where it does not.Worth naming: the arm used to take precedence over everything, on the reasoning that
staticis a deliberate act by the operator and preferring Composio would take away the hatch they configured. What that missed is what the hatch confers. It preserved their configuration by handing them a green tile and no capability.CONNECTION_PROVIDERSstops being a source of offerable tiles.buildGridProvidersno longer appends every local tile the backend catalog missed. It keeps its metadata role — brand colours, the idswell_knownkeys, the Composio slug each maps to, thex/twitteralias fold — so a provider the backend catalog does not carry no longer appears solely because the console has a logo for it.A provider already connected natively keeps saying so. The tail that used to be "everything we have metadata for" is now "everything the host reports as connected", catalog or no catalog.
via: ["native"]and Disconnect stay, per the issue.providerIdgained a middle fallback — local tile, then the host's own spelling fromGET …/connections, then the slug — soDELETE …/connections/{provider}can still name a connected provider the console has no tile for, now that its row is the only reason it has a tile.An honest empty state. The native fallback used to paper over a host whose catalog is genuinely empty; without it, "No provider in All." would read as a broken filter rather than as a host with nothing to offer.
docs/modules/server/README.mdno longer describes the native route as a live console path, and thestaticrow of thecredentialSourcetable says what the console does with it now.The host routes are untouched. This is the console declining to offer a path, not the path being removed — settling #396 makes the offer honest again, and reinstating it is one arm in
connectRoute.One correction to the issue's picture of the blast radius
The issue (and my first draft of the new spec) assumed a host without Composio serves an empty catalog. It does not:
GET …/composioanswers200even in a build carrying nocomposiofeature —inBuild: false,catalogSource: "fallback", and a built-in starter list of eight slugs. So on a typical self-hosted instance this removes five tiles (Dropbox, Stripe, HubSpot, X, LinkedIn — theCONNECTION_PROVIDERSentries that list does not carry), not eleven, and the eight the host does offer stay and say "not available here". All eleven go only on a host that answers no catalog at all. The code comments and the new spec say so; this is also why the empty state is rarer than it looks and is exercised through a stubbed/composio.How it was verified
Beyond the unit and Playwright suites, the defect and the fix were both driven end-to-end against the real binary with the native hatch genuinely open — a local stand-in provider behind
OPENCOMPANY_OAUTH_SLACK_AUTHORIZE_URL/_TOKEN_URL, a realOPENCOMPANY_OAUTH_STATE_SECRET, and a company declaring[[connection]] slack— so the handshake under discussion actually completes rather than being described:oauth/slack, tile green "Acme Workspace"Then Disconnect from the post-fix bundle: the host went
connected: true, via: ["native"]→connected: false, via: [], the stand-in provider logged the revoke, and Slack fell back to "not available here" with no Connect — whilecredentialSourcestayedstatic. Which is the whole point: the route still works, and the page no longer invites it.Commands run
npm run typecheck/npm run typecheck:e2e— cleannpx vitest run— 603 passed (50 files)PW_BASE_URL=… PW_STORAGE_STATE=… npx playwright testagainst an isolated host — 127 passed, 10 skipped (the live-brain lane), including the four new specs and the three pre-existing connections specsTests
frontend/test/e2e/connections-native-not-offered.spec.tsis new: the console-only tiles are gone, no Connect is offered where none can confer anything, a natively connected provider keeps its tile and its Disconnect, and an empty catalog explains itself. Two of the four stub one host answer each — a stored native credential, and a host with no catalog — because this harness can produce neither; both are pinned on the merge itself intest/unit/provider-grid.test.ts, and the spec header says which is which and why.connection-route.test.tsswaps its two{ kind: "native" }assertions for the routes that replace them, and adds a sweep asserting no tile routes natively under any tier/reach combination.provider-grid.test.tsreplaces "keeps every local tile when the host offers no Composio catalog" — the assertion that was the defect — with the connected-tail rules.Notes for review
armTourResume(issue #300) loses its only caller: it existed for the full-page navigation the native Connect performed, and the Composio sign-in that remains opens a tab instead. The read half stays wired —TourControllerstill honours a marker, and the host still redirects an in-flight handshake back to/connections?connected=…, so a marker written by an older bundle is still consumed. I kept the function with a note rather than deleting the machinery, which would have been a second, unrelated change; say the word if you would rather it went.Closes #822.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Updates
Tests