feat(integrations): let a host publish adapter-only connectors it drives itself - #3450
Merged
Merged
Conversation
…ves itself Hiding adapter-only connectors from every lookup removed the connector definitions out from under hosts that do implement the adapter. veryfront-api ships a Salesforce client (instance_url resolution, auth retry) and names salesforce in its promoted set, but 0.1.1209 stopped returning the connector, so tool names no longer resolved and OAuth connection binding stopped being required. The integration stayed connectable with no working tools. The scaffolding guard itself is correct and stays exactly where it was. `veryfront init` and the template loader must keep refusing these: generated routes run on the generic OAuth runtime, which is precisely what cannot serve them. The generic OAuthProvider constructor guard is untouched too. Catalog lookup is the seam that was too narrow, so it gets its own predicate. `getConnector`, `listConnectors` and `getIcon` now use `isCatalogVisibleIntegration`, which additionally honours `VERYFRONT_HOST_ADAPTER_INTEGRATIONS`. A host names each connector it actually implements; blanket values are ignored, so a sweep can never widen the scaffolding surface by accident. Tested: full unit suite 3811 passed, 0 failed; verify:quick exit 0. The CLI scaffolding tests that pin the guard pass unmodified, which is the point.
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
📝 WalkthroughWalkthroughHost-declared adapter integrations can be explicitly enabled for catalog lookup and connector listing. Scaffolding visibility remains unchanged. Blanket environment values do not enable integrations. Tests cover feature flags and catalog APIs. ChangesHost adapter catalog visibility
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant HostEnvironment
participant FeatureFlags
participant IntegrationCatalog
HostEnvironment->>FeatureFlags: provide explicit adapter names
FeatureFlags->>IntegrationCatalog: check catalog visibility
IntegrationCatalog->>IntegrationCatalog: filter visible integrations
IntegrationCatalog-->>HostEnvironment: return declared connector
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/integrations/index.test.ts`:
- Around line 57-62: Add an assertion in the host-declared integration test
around getConnector("salesforce") that getIcon("salesforce") returns a defined
icon, while retaining the existing connector and undeclared-adapter assertions.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9cfaead4-63e6-4452-b470-d2dd166a4f60
📒 Files selected for processing (4)
src/integrations/feature-flags.test.tssrc/integrations/feature-flags.tssrc/integrations/index.test.tssrc/integrations/index.ts
Comment on lines
+57
to
+62
| Deno.env.set(HOST_ADAPTER_INTEGRATIONS_ENV, "salesforce"); | ||
| try { | ||
| assertEquals(getConnector("salesforce") !== undefined, true); | ||
| assertEquals(getConnectorNames().includes("salesforce"), true); | ||
| // Adapter-only and not declared by the host, so still absent. | ||
| assertEquals(getConnector("pipedrive"), undefined); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Test host-declared icon lookup.
This test does not assert getIcon("salesforce"). A regression that keeps getIcon on generic visibility passes this test while declared host adapters cannot load their icon.
Proposed test update
try {
assertEquals(getConnector("salesforce") !== undefined, true);
+ assertEquals(getIcon("salesforce"), icons.salesforce);
assertEquals(getConnectorNames().includes("salesforce"), true);As per coding guidelines, "When public behavior changes, update the relevant documentation, examples, generated references, command help, and tests."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Deno.env.set(HOST_ADAPTER_INTEGRATIONS_ENV, "salesforce"); | |
| try { | |
| assertEquals(getConnector("salesforce") !== undefined, true); | |
| assertEquals(getConnectorNames().includes("salesforce"), true); | |
| // Adapter-only and not declared by the host, so still absent. | |
| assertEquals(getConnector("pipedrive"), undefined); | |
| Deno.env.set(HOST_ADAPTER_INTEGRATIONS_ENV, "salesforce"); | |
| try { | |
| assertEquals(getConnector("salesforce") !== undefined, true); | |
| assertEquals(getIcon("salesforce"), icons.salesforce); | |
| assertEquals(getConnectorNames().includes("salesforce"), true); | |
| // Adapter-only and not declared by the host, so still absent. | |
| assertEquals(getConnector("pipedrive"), undefined); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/integrations/index.test.ts` around lines 57 - 62, Add an assertion in the
host-declared integration test around getConnector("salesforce") that
getIcon("salesforce") returns a defined icon, while retaining the existing
connector and undeclared-adapter assertions.
Source: Coding guidelines
Merged
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.
Problem
Hiding adapter-only connectors from every lookup removed the connector definitions out from under hosts that do implement the adapter.
veryfront-apiships a full Salesforce client (usecases/integrations/salesforce/api-client.ts— resolvesinstance_urlfrom the token bundle, handles auth retry) and namessalesforcein its promoted set. After0.1.1209:getConnector('salesforce')returnsundefinedeven with the host's flag setisKnownIntegrationToolName('salesforce__…')isfalse, so tool names stop resolvingrequiresOAuthConnectionBinding('salesforce')silently becomesfalseThe integration stays connectable and the adapter still works, but it has no tools. A user who connected Salesforce and called a case tool two weeks ago cannot do so now.
What is not changing
The scaffolding guard is correct and stays exactly where it is. I initially tried widening
isExperimentalIntegrationEnabledand it brokecli/commands/init/catalog.test.tsandcli/templates/integration-loader.test.ts— which is the guard doing its job. Generated routes run on the generic OAuth runtime, and that is precisely what cannot serve these connectors.Unchanged, still refusing adapter-only connectors:
cli/commands/init/catalog.ts,cli/templates/integration-loader.ts,cli/mcp/tools/catalog-tools.tssrc/oauth/providers/common.tsvisible-service filteringOAuthProviderconstructor guard onruntimeSupport: "provider-adapter-required"Those CLI tests pass unmodified in this PR, which is the clearest evidence the seam is right.
The change
Catalog lookup was the part that was too narrow, so it gets its own predicate.
getConnector,listConnectorsandgetIconnow useisCatalogVisibleIntegration, which isisVisibleIntegrationplusisHostAdapterIntegration. The latter reads a newVERYFRONT_HOST_ADAPTER_INTEGRATIONSenv var.Deliberate properties:
1,true,all,*) are ignored, so a sweep can never widen the surface by accident. Covered for all four forms.falsefor anything not inINTEGRATIONS_REQUIRING_PROVIDER_ADAPTER, so it cannot become a second general-purpose visibility flag.Tests
pipedrive) is notgetConnector("salesforce")andgetConnectorNames()include it under the declarationEach confirmed to actually execute rather than silently skip.
Verification
deno task test:unit: 3811 passed, 0 faileddeno task verify:quick: exit 0 (full 22-task chain)Follow-up
Consumers still need to opt in.
veryfront-apiwill setVERYFRONT_HOST_ADAPTER_INTEGRATIONS=salesforceand restore its catalog expectations once this publishes. The other 11 adapter-only connectors stay hidden everywhere until some host declares an adapter for them.Summary by CodeRabbit
all,*,true, and1from enabling integrations.