LLM provider management: write tools (BYOM)#434
Conversation
e3181cc to
5620bcd
Compare
…default set/reset) Adds custom (BYOM) LLM provider management across SDK, MCP, and CLI: create, update (full configuration replacement), delete, active-status toggle, and organization default set/reset — the write counterpart to the provider discovery surface. Secret handling: provider configuration is supplied only via a local JSON file path, never inline, so it never becomes a logged tool argument; the create and update mutations never select configuration back (secrets are never returned); and file/parse errors report only the path and a structural reason. Updates send a full replacement configuration and preserve stored secrets for any value left as the redaction placeholder returned on read. Owner-scoped operations (active-status, default set/reset) authorize against the credential's own organization context, so they require a service-account credential bound to that organization; create/update/delete authorize against the passed organization UUID. Deletes require confirmation; CLI create/update are gated on the read-access probe (clean only when ok and no problem).
5620bcd to
7b9a81d
Compare
adriannoes
left a comment
There was a problem hiding this comment.
Summary
Checkout+tests on tip 7b9a81d (87 provider unit tests + ruff green; CI lint/test/skills green). The write surface itself (file-only secrets, omitted configuration on create/update, CLI clean-gate, parity/registry) looks solid against #415. One point-of-use docs gap needs a fix before agents reliably create non-openai BYOM providers: the kebab vs snake vendor vocabularies are listed on different tools with no bridge.
What worked well
- Secret handling is coherent end-to-end: path-only input, no
configurationin create/update selection sets, structural-only file errors, and tests that secrets never appear in parse error strings. - CLI create/update clean-gate (
okand noproblem) matches the #411/#415 contract; probe docs state it explicitly. - Owner-scoped auth (service account) and UUID vs numeric id quirks are documented in MCP tool docstrings and
docs/mcp/tools/llm-providers.md.
Required before merge
- Document that
configuration.provider(kebab, e.g.oracle-oci) is not interchangeable withProviderName/get_available_ai_models(provider_namesnake, e.g.oracle_oci). Live create with snake fails as invalid adapter; models reject kebab. Fix in the create tool docstring and the configuration section ofdocs/mcp/tools/llm-providers.md(optional one-line cross-ref on the models tool).
Also noted
- The AI-agents skill BYOM paragraph lists owner-scoped ops (
set_llm_provider_active_status, default set/reset) without restating the service-account requirement or UUID vs numeric split. Tool docstrings already cover this; optional skill polish only (not on the #415 acceptance path of validate → create/list → behaviorproviderId).
Review path
- Checked out PR tip in a worktree, ran provider-focused unit tests and ruff; CI green on tip.
- Live checks confirmed dual vendor vocabularies (config reads
oracle-oci;ProviderNameis snake-only; create with snake → Invalid adapter). - Security and architecture passes found no medium+ exploit or facade bypass; related KB probe-gate work stays in #435/#436 after this PR.
| Configuration is read from a local JSON file — never inline — so secrets | ||
| never become a logged argument, and the created provider is returned | ||
| without its configuration. The `provider` key in the file selects the | ||
| vendor (e.g. `openai`, `anthropic`, `amazon-bedrock`, `azure-openai`, |
There was a problem hiding this comment.
The create path and the models lookup use different vendor string forms, and the docs list each without saying they are not interchangeable.
configuration.provider in the JSON file for create_llm_provider / update_llm_provider expects kebab-case values such as oracle-oci and amazon-bedrock. Live create rejects snake_case forms as an invalid adapter.
get_available_ai_models takes provider_name in snake_case (oracle_oci, amazon_bedrock, …). Kebab-case is rejected there.
Please add an explicit note in the create tool docstring and in docs/mcp/tools/llm-providers.md (configuration section) that the two vocabularies are surface-specific and must not be copied across. A one-line cross-reference from the models tool Args would help agents avoid a failed round-trip when wiring BYOM.
There was a problem hiding this comment.
Good catch — fixed in 45c013f. The two vocabularies are now explicitly bridged, with the non-interchangeability stated at each point of use:
create_llm_providerdocstring: notes the hyphenatedproviderstrings are file-specific and thatget_available_ai_modelstakes snake_case, with the invalid-adapter consequence.get_available_ai_modelsArgs: one-line cross-ref back to theconfiguration.providerhyphenated form.docs/mcp/tools/llm-providers.md(configuration section): a dedicated "surface-specific and not interchangeable" note listing both forms and noting single-word vendors (openai,anthropic,custom) are identical on both.
I also extended it to the CLI, since it's the same footgun there: the shared --config-file help (create/update) and the models --provider-name help now carry the same cross-reference, and the help golden is regenerated.
…lm-provider-writes
…ider config The configuration.provider key (create/update) uses hyphenated vendor strings (amazon-bedrock, oracle-oci) while get_available_ai_models takes provider_name in snake_case (amazon_bedrock, oracle_oci). The two are not interchangeable; a value copied across surfaces is rejected. Note this across both surfaces: the MCP create docstring, models tool Args, and the LLM providers doc; and the CLI --config-file help, the models --provider-name help, and the regenerated help golden.
|
Thanks for the thorough pass @adriannoes. Required (kebab vs snake vocabulary): Done in 45c013f, and replied inline with specifics. Bridged at every point of use across both surfaces — MCP ( Also noted (skill BYOM paragraph): I'm going to leave this one as-is. The service-account requirement and the UUID-vs-numeric split already live where an agent hits them at call time — the tool docstrings and |
Closes #415. Final phase of milestone 19 (AI Agents: Knowledge Bases, Capabilities & Model Selection); the write counterpart to the provider discovery surface (#411).
Motivation
The discovery phase (#411) made LLM providers readable but not manageable. Bringing your own model (BYOM) still required the Pipefy UI: there was no way to create a custom provider, rotate its credentials, toggle it, or set the organization default from an agent or script. This phase closes that gap while keeping provider secrets out of arguments, logs, errors, and responses.
What ships
Six write tools across SDK → MCP → CLI, plus docs and the AI-agents skill:
create_llm_providerpipefy ai-provider createupdate_llm_providerpipefy ai-provider updatedelete_llm_providerpipefy ai-provider deleteset_llm_provider_active_statuspipefy ai-provider set-active-statusset_default_llm_providerpipefy ai-provider default setreset_default_llm_providerpipefy ai-provider default resetProven contract
Verified against the live GraphQL schema and traced through the full delegation chain (GraphQL entry → downstream credential-validating service), per-argument:
configuration_file_path) and CLI (--config-file) accept the provider configuration only via a local JSON file, never inline — so it never becomes a logged tool argument. Theproviderkey selects the vendor; vendor/model membership and credential validity are validated server-side (a live credential test call), so the toolkit treats the object as opaque rather than coupling to the vendor schema.create/updatedeliberately omitconfigurationfrom their selection sets even though the payload exposes it; the response carries onlyid/name/type/active/organizationDefault. File/parse errors report only the path and a structural reason, never file contents.configurationis required on every update. A caller can fetch the (redacted) configuration, edit the non-secret fields, and resubmit: any value left as the__REDACTED__placeholder is stripped upstream and the stored secret is preserved — confirmed live (an update with the placeholder succeeds; the backend used the stored key, not the literal placeholder). Supplying a new real value rotates the secret.create/update/deletetake the organization UUID;set-default/resettake the numeric organization id;set-active-statustakes no organization argument;set-defaultrequires exactly one ofprovider_id/system_provider_id(enforced client-side and downstream).manage_ai_providersorganization permission and an eligible plan — distinct frommanage_ai_agents. The read-access probe proves read only, so a green probe does not imply write entitlement.set-active-status,set-default,reset, and the organization-ownerget_default_llm_providerauthorize against the credential's own organization context. A service account is bound to one organization and supplies that context; a bare personal token is denied regardless of the organization passed — even when the same token can create/update/delete providers there (those authorize against the passed UUID). Documented in the tool docstrings anddocs/mcp/tools/llm-providers.md.Behavior boundaries
okand carries noproblem(a presentproblemis partial denial, never full access); the 2. LLM provider discovery: read tools and access probe #411 probe docs now state this clean-gate contract explicitly.confirm; CLI prompt unless--yes).Known upstream issue
Provider credential-validation errors are forwarded from the backend and can echo a masked copy of the submitted credential plus an internal
error_codeblob. This originates upstream; the toolkit surfaces the classified message verbatim (no client-side scrub, by decision) and the upstream fix is tracked separately (here)Docs and skills touched
docs/mcp/tools/llm-providers.md(writes, secret handling, the update flow, the probe clean-gate contract, service-account requirement),docs/parity.md, README / packages/mcp/README tool counts (reconciled to 182), the AI-agents skill (BYOM provider management flow), and the registry.Testing
Unit tests assert configuration never appears in responses, logs, or errors; the XOR rule; full-replacement update; the redaction placeholder passthrough; confirm-delete; and the probe clean-gate. Live end-to-end against a lab organization:
__REDACTED____REDACTED____REDACTED__; model changed; stored secret preserved (no re-auth failure)PERMISSION_DENIED(documented auth boundary)