Add Atlas Cloud agent provider - #581
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
EtienneLescot
left a comment
There was a problem hiding this comment.
Thanks for this — the Atlas registration itself is clean and consistent with how openrouter is modelled, and it lands in every union/registry that matters (AgentCapabilityProvider, AgentModelProvider, AgentProviderId, the runtime AGENT_MODEL_PROVIDERS) plus all three normalizeAgentProviderId copies. I ran tsx --test tests/unit/agent-provider-atlascloud.test.ts locally: 3/3 pass.
Requesting changes on one item only: the PR also changes credential resolution for every existing provider, and that part has a regression.
Blocking
readProviderEnvironmentSecret bypasses the disabled-providers gate. The runtime now falls back to env keys for the actual API key, but unlike AgentProviderService.readEnvironmentCredential (agent-provider-service.ts:464) it never consults n8n.agent.disabledProviders. Concretely: OPENAI_API_KEY is in the environment, user hits Disconnect on OpenAI → the secret is deleted, openai is added to the disabled set, and listProviderConnectionStates reports connected: false. Before this PR the runtime refused with "Missing API key for OpenAI API"; after it, getProviderRuntimeConfig returns ready: true and the workbench silently bills against the env key the user just explicitly disabled. Same for anthropic / google / mistral / openrouter. Details inline.
The broader env fallback is a reasonable improvement in itself (the provider service already treats env credentials as "connected") — it just needs the same gate, and it deserves a line in the PR description since it is not Atlas-specific.
Non-blocking, worth folding in
- The controller's
envKeysmap is a third copy of the credential table and is still missingminimax/minimax-token-plan/copilot-proxy.agent-provider-settings.tsis imported by both files and would be a cycle-free home for it. n8n.agent.providerenum inpackages/vscode-extension/package.json:316lists all ten existing providers but notatlascloud. Nothing reads it (the value lives inglobalState), so it is cosmetic — but it is the one place a reader looks for the supported set.scripts/run-provider-stream-v3-integrations.mjs:8keeps its own provider list and drives the matrix viaN8N_AGENT_TEST_PROVIDERSper provider. Atlas was added to the four test files but not here, sonpm run test:integration:providerswill never exercise it.
Two discussion threads inline: one asking for a live end-to-end pass before merge, one on reasoning effort for the DeepSeek default.
| const apiKey = await this._context.secrets.get(getAgentProviderSecretKey(normalizedProvider)); | ||
| const baseUrl = this.resolveProviderRuntimeBaseUrl(normalizedProvider, settings.baseUrl); | ||
| const apiKey = await this._context.secrets.get(getAgentProviderSecretKey(normalizedProvider)) | ||
| || this.readProviderEnvironmentSecret(normalizedProvider); |
There was a problem hiding this comment.
Blocking. This env fallback ignores the disabled-providers state that the rest of the credential surface honours.
AgentProviderService.readEnvironmentCredential guards with if (this.getDisabledProviders().has(provider)) return undefined; (agent-provider-service.ts:464), and disconnectProvider deletes the stored secret and adds the provider to n8n.agent.disabledProviders. readProviderEnvironmentSecret has no such guard, so after a Disconnect the UI shows connected: false while the runtime happily runs on the env key.
Repro: set OPENAI_API_KEY, Disconnect OpenAI in Settings > Agent Providers, then send a workbench prompt. Pre-PR: "Missing API key for OpenAI API". Post-PR: the run succeeds.
The controller already holds _context.globalState, so the smallest fix is to read DISABLED_PROVIDERS_STATE_KEY here and return undefined for a disabled provider — or to route the lookup through AgentProviderService so there is one code path. A unit test asserting "disabled provider does not pick up its env key" would lock it down.
| google: ['GOOGLE_GENERATIVE_AI_API_KEY', 'GEMINI_API_KEY', 'GOOGLE_API_KEY', 'GEMINI_LLM_API_KEY', 'GOOGLE_LLM_API_KEY'], | ||
| mistral: ['MISTRAL_API_KEY', 'MISTRAL_LLM_API_KEY'], | ||
| openrouter: ['OPENROUTER_API_KEY', 'OPENROUTER_LLM_API_KEY'], | ||
| atlascloud: ['ATLASCLOUD_API_KEY', 'ATLAS_CLOUD_API_KEY'], |
There was a problem hiding this comment.
This map is a second source of truth for AGENT_PROVIDER_DEFINITIONS[].envKeys. The PR fixes the existing drift for five providers (good catch on the *_LLM_API_KEY aliases), but then leaves new drift: minimax (MINIMAX_API_KEY), minimax-token-plan (MINIMAX_TOKEN_PLAN_API_KEY) and copilot-proxy (COPILOT_GITHUB_TOKEN, GH_TOKEN, GITHUB_TOKEN) are absent, so env credentials now work for some api-key providers and silently not for others.
agent-provider-service.ts imports from this file, so importing AGENT_PROVIDER_DEFINITIONS back would cycle. agent-provider-settings.ts is already imported by both and would be a cycle-free home for the shared table.
|
|
||
| private resolveProviderRuntimeBaseUrl(provider: string, configuredBaseUrl?: string): string | undefined { | ||
| if (provider === 'atlascloud') { | ||
| return this.readFirstEnvironmentValue(['ATLASCLOUD_BASE_URL', 'ATLAS_CLOUD_BASE_URL', 'ATLASCLOUD_API_BASE', 'ATLAS_CLOUD_API_BASE']) |
There was a problem hiding this comment.
Two things here:
configuredBaseUrlis dropped entirely foratlascloud, and env ranks above explicit config — the inverse of every other provider (providerConfig.baseUrl || default). Not reachable through the UI today sinceproviderNeedsBaseUrlInputis openai-compatible-only, but it is a trap for whoever wires up per-provider base URLs later.configuredBaseUrl || env || ATLAS_CLOUD_DEFAULT_BASE_URLkeeps the precedence people expect.- This alias list is written twice — here and as
baseUrlEnvKeysinAGENT_PROVIDER_DEFINITIONS.atlascloud. Same shared-table point as the env keys above.
| : provider === 'google' | ||
| ? providerConfig.baseUrl || 'https://generativelanguage.googleapis.com/v1beta/openai' | ||
| : providerConfig.baseUrl; | ||
| : provider === 'atlascloud' |
There was a problem hiding this comment.
Nit: unreachable in practice. resolveProviderRuntimeBaseUrl never returns a falsy value for atlascloud (env or default), so providerConfig.baseUrl is always set by the time it gets here. Harmless as defence in depth, but it is the third place https://api.atlascloud.ai/v1 is hardcoded (here, ATLAS_CLOUD_DEFAULT_BASE_URL in this file, and defaultBaseUrl in the provider definitions) — worth collapsing to one exported constant. Fixing the precedence in resolveProviderRuntimeBaseUrl also makes this branch consistent rather than redundant.
| const baseUrl = provider === 'openai-compatible' ? configuredBaseUrl : this.readEnvironmentBaseUrl(provider) || definition.defaultBaseUrl; | ||
|
|
||
| if ((definition.requiresApiKey || provider !== 'openai-compatible') && !apiKey && definition.authKind !== 'none') { | ||
| if (provider !== 'atlascloud' && (definition.requiresApiKey || provider !== 'openai-compatible') && !apiKey && definition.authKind !== 'none') { |
There was a problem hiding this comment.
This condition was already dense; the provider !== 'atlascloud' prefix makes the intent hard to recover. A named set reads better and documents why:
const PROVIDERS_WITH_PUBLIC_MODEL_CATALOG = new Set<AgentModelProvider>(['atlascloud']);Side effects of the bypass worth a thought: a user with no Atlas key can now pick a model from a live list and only discover at run time that requiresApiKey: true blocks the run, and discovery also fires for an Atlas provider the user explicitly disconnected (user-initiated, so not a beacon — but inconsistent with readEnvironmentCredential's disabled gate).
| return this.fetchJsonModels(modelsUrl, apiKey ? { Authorization: `Bearer ${apiKey}` } : {}); | ||
| } | ||
|
|
||
| private async fetchAtlasCloudModels(): Promise<string[]> { |
There was a problem hiding this comment.
This ignores both apiKey and the baseUrl computed at line 416, so an ATLASCLOUD_BASE_URL override (a self-hosted or regional gateway) still queries api.atlascloud.ai for its model list. Passing the resolved base URL, and an Authorization header when a key exists, costs nothing and keeps the override honest.
No try/catch or timeout is fine — that matches fetchJsonModels, and both call sites already use .catch(() => []).
This is the only genuinely new logic in the PR and it is the one thing not covered by a behavioural test: the type === 'text' filter, model-over-id preference, dedupe, sort, and !response.ok → []. A stubbed fetch in the new unit file would cover all five cheaply.
| description: 'ATLASCLOUD_API_KEY', | ||
| defaultModel: 'deepseek-ai/deepseek-v4-pro', | ||
| defaultBaseUrl: ATLAS_CLOUD_DEFAULT_BASE_URL, | ||
| baseUrlEnvKeys: ['ATLASCLOUD_BASE_URL', 'ATLAS_CLOUD_BASE_URL', 'ATLASCLOUD_API_BASE', 'ATLAS_CLOUD_API_BASE'], |
There was a problem hiding this comment.
Adding baseUrlEnvKeys to the shared definition shape is the right move — but then resolveProviderRuntimeBaseUrl in agent-runtime-controller.ts hardcodes the same four names instead of reading them from here. Please make one of the two the source of truth. Also worth a unit test for the precedence and the trailing-slash trim in readEnvironmentBaseUrl.
| id: 'atlascloud', | ||
| label: 'Atlas Cloud', | ||
| envKeys: ['ATLASCLOUD_API_KEY', 'ATLAS_CLOUD_API_KEY'], | ||
| model: process.env.ATLASCLOUD_MODEL || process.env.ATLAS_CLOUD_MODEL || process.env.N8N_AGENT_TEST_ATLASCLOUD_MODEL || 'deepseek-ai/deepseek-v4-pro', |
There was a problem hiding this comment.
Could you run one live end-to-end pass against Atlas before merge, and paste the output here?
N8N_AGENT_TEST_PROVIDERS=atlascloud npx tsx --test packages/vscode-extension/tests/integration/provider-stream-v3.integration.test.tsThe validation notes in the description confirm the model catalog returns 437/121 models, which exercises discovery — but nothing in the PR exercises an actual agent run. That is the part that tends to break for a new OpenAI-compatible endpoint, and the workbench depends on it:
- tool-calling round-trips through the shim (the whole point of the provider),
- reasoning content leaking into assistant text —
deepseek-ai/deepseek-v4-prois a thinking model reached through an OpenAI-compatible shim, andagent-runtime-controllerhas middleware specifically for non-final assistant phases, - streaming vs tool-call interaction, cf. the google-specific
shouldDisableModelStreamingForToolCalling.
Ideally also tests/integration/tool-usage-workflow-authoring.integration.test.ts and native-mcp-agent-routing.integration.test.ts, since you added Atlas cases to those matrices too. Note that scripts/run-provider-stream-v3-integrations.mjs has its own provider list and was not updated, so the orchestrator will skip Atlas — hence the manual N8N_AGENT_TEST_PROVIDERS above.
| }); | ||
|
|
||
| test('Atlas Cloud provider does not opt into provider-specific reasoning knobs', () => { | ||
| assert.equal(getReasoningCapability('atlascloud', 'deepseek-ai/deepseek-v4-pro').supported, false); |
There was a problem hiding this comment.
Worth opening this up rather than just asserting the status quo: deepseek-ai/deepseek-v4-pro is the default model and, as a DeepSeek reasoning model, it does have a thinking budget — so pinning supported: false means Workbench users get whatever the endpoint defaults to, with no UI control and no include_reasoning handling.
Atlas exposes an OpenAI-compatible surface, so the existing openrouter-reasoning strategy in agent-provider-capabilities.ts is probably a drop-in:
modelKwargs: { reasoning: { effort }, include_reasoning: true }Two questions:
- Does the Atlas
/v1/chat/completionsendpoint accept OpenAI-stylereasoning: { effort }(or DeepSeek-stylethinking) for the DeepSeek models, and does it echo reasoning back in a separate field or inline incontent? - If it does not, is
falsehere a deliberate "ship the plumbing first, reasoning later" call?
Either answer is fine by me — I would just rather it be a documented decision than an accident, since it also determines whether reasoning text ends up rendered as assistant output. A follow-up issue is enough if the answer is (2).
| assert.ok(providerService.includes("'ATLASCLOUD_API_KEY', 'ATLAS_CLOUD_API_KEY'"), 'Provider service must support both Atlas API key env names'); | ||
| assert.ok(providerService.includes("normalized === 'atlas' || normalized === 'atlas-cloud'"), 'Provider service must normalize Atlas aliases'); | ||
|
|
||
| assert.ok(runtimeController.includes("'atlascloud'"), 'Runtime registry must include the Atlas provider id'); |
There was a problem hiding this comment.
Source-text assertions are an established pattern in this repo (agent-workbench-html.test.ts:222 does the same), so no objection in principle. But this one passes on any occurrence of 'atlascloud' anywhere in a ~4900-line file, so it asserts close to nothing — the display-name and alias assertions below it already cover the intent. The alias test above and the getReasoningCapability assertion are the valuable ones here.
The coverage I would rather have in exchange: fetchAtlasCloudModels payload handling (see the comment on agent-provider-service.ts), readEnvironmentBaseUrl precedence, and the disabled-provider case for the new env-secret fallback.
|
Thanks for the detailed review. I pushed follow-up commit
Validation run:
I also tried the broader local checks. Root |
|
Small follow-up pushed in Re-ran |
What does this PR do?
Adds Atlas Cloud as an Agent Workbench LLM provider using the existing OpenAI-compatible ChatOpenAI runtime path.
atlascloudprovider withatlas/atlas-cloudaliaseshttps://api.atlascloud.ai/v1anddeepseek-ai/deepseek-v4-prodefaultsATLASCLOUD_API_KEY/ATLAS_CLOUD_API_KEYand Atlas base URL aliasesFollow-up after review
n8n.agent.providerenum and provider stream integration runnermodeloverid, dedupe/sort, env precedence, and trailing-slash trimmingRelated issue (if any): #
Validation:
npm testfrompackages/vscode-extension(147 passed)npm run buildfrompackages/vscode-extensionN8N_AGENT_TEST_PROVIDERS=atlascloud npx tsx --test packages/vscode-extension/tests/integration/provider-stream-v3.integration.test.ts(PROVIDER_STREAM_OK)npm run docs:api -- --dry-runnpm run typecheckfromdocsafter installing docs dependencies locallygit diff --checkNotes:
npm run buildstill stops before project compilation at the upstream lockfile supply-chain policy check:xlsx@0.20.2has no tarball integrity entry. I did not modifypnpm-lock.yaml.docsbuild still fails on existing Docusaurus/Mermaid SSR pages withuseColorMode is called outside the <ColorModeProvider>for contribution docs paths; this is unrelated to the Atlas provider files changed here.include_reasoning/reasoning payloads would alter the runtime request contract and should be handled as a separate provider capability follow-up once the endpoint contract is confirmed.