fix(copilot): strip platform-managed apiKey on hosted-tool blocks in edit_workflow#5217
Conversation
…edit_workflow preValidateCredentialInputs only stripped apiKey for hosted LLM models (getHostedModels). Agent-authored apiKey on hosted-tool blocks (Fal video/image, etc.) slipped through, disabling hosted-key injection and misleading the agent into telling users to bring a key. Generalize the strip to key off tool.hosting — the same canonical signal injectHostedKeyIfNeeded uses at execution. Resolve the block's active tool via its tools.config.tool selector, honor the per-provider enabled gate, and strip the exact hosting.apiKeyParam field (no hardcoded 'apiKey' assumption). Surfaces the existing non-fatal note to the agent. Self-hosted and non-hosted providers untouched.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR SummaryMedium Risk Overview
Tests add hosted-tool scenarios (provider routing, edit-only Reviewed by Cursor Bugbot for commit 95a449f. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
@greptile review |
…-key-mothership # Conflicts: # apps/sim/lib/copilot/tools/server/workflow/edit-workflow/validation.test.ts
Greptile SummaryThis PR fixes a bug where the copilot intermittently authored platform-managed API key fields (e.g.
Confidence Score: 4/5The fix is safe to merge — it only adds a new stripping pass that runs after the existing hosted-model pass, leaves self-hosted deployments and non-hosted providers completely untouched, and is guarded by isHosted throughout. The core logic correctly follows tool.hosting as the canonical signal and properly merges existing subblock state for edit ops on top-level blocks. Two minor concerns: a tool selector that returns a non-string is silently treated as a no-op miss rather than an explicit guard, and the existingBlock lookup from workflowState is performed twice for edit ops. Neither affects correctness in the described scenarios, but they leave small silent-miss edge cases unguarded. Both changed files look good. The test file covers the key scenarios thoroughly. The only area worth a second look is the tool-selector null-return path in collectHostedToolApiKeyInput. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[preValidateCredentialInputs] --> B{op.params.type?}
B -- yes --> C[getBlock blockConfig]
C --> D[collectHostedApiKeyInput\nhosted LLM models]
C --> E{edit op +\nworkflowState?}
E -- yes --> F[merge existing subBlock values\ninto toolParams]
E -- no --> G[toolParams = op.params.inputs]
F --> H[collectHostedToolApiKeyInput]
G --> H
H --> I{blockConfig.tools?}
I -- no --> Z[skip]
I -- yes --> J{toolSelector exists?}
J -- yes --> K[toolSelector toolParams\nresolve active toolId]
J -- no --> L[all tools.access as candidates]
K --> M[getTool toolId]
L --> M
M --> N{tool.hosting?}
N -- no --> Z
N -- yes --> O{hosting.enabled gate?}
O -- fail --> Z
O -- pass / no gate --> P[add hosting.apiKeyParam\nto managedFieldIds]
P --> Q{field present in inputs?}
Q -- yes --> R[push to hostedApiKeyInputs]
Q -- no --> Z
R --> S[structuredClone ops\nstrip field + push error]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[preValidateCredentialInputs] --> B{op.params.type?}
B -- yes --> C[getBlock blockConfig]
C --> D[collectHostedApiKeyInput\nhosted LLM models]
C --> E{edit op +\nworkflowState?}
E -- yes --> F[merge existing subBlock values\ninto toolParams]
E -- no --> G[toolParams = op.params.inputs]
F --> H[collectHostedToolApiKeyInput]
G --> H
H --> I{blockConfig.tools?}
I -- no --> Z[skip]
I -- yes --> J{toolSelector exists?}
J -- yes --> K[toolSelector toolParams\nresolve active toolId]
J -- no --> L[all tools.access as candidates]
K --> M[getTool toolId]
L --> M
M --> N{tool.hosting?}
N -- no --> Z
N -- yes --> O{hosting.enabled gate?}
O -- fail --> Z
O -- pass / no gate --> P[add hosting.apiKeyParam\nto managedFieldIds]
P --> Q{field present in inputs?}
Q -- yes --> R[push to hostedApiKeyInputs]
Q -- no --> Z
R --> S[structuredClone ops\nstrip field + push error]
|
Greptile SummaryThis PR generalizes the existing hosted-LLM apiKey stripping in
Confidence Score: 4/5Safe to merge — the change fixes a real bug on the hosted platform and is well-covered by tests for the primary paths. The two gaps (unguarded enabled() predicate, no state enrichment for nested-edit ops) are edge cases that don't affect the common add-operation scenario this fix targets. The core fix correctly mirrors injectHostedKeyIfNeeded's logic and is exercised by 7 new tests covering the key variants. The enabled() predicate in collectHostedToolApiKeyInput is not wrapped in try/catch unlike the toolSelector call, so a misbehaving predicate could surface as an unhandled error on any edit_workflow call touching a tool-hosted block. Nested edit operations inside loop/parallel blocks also don't benefit from the workflowState enrichment added for main blocks, leaving a blind spot when a child's provider isn't restated in the edit op. validation.ts — specifically the collectHostedToolApiKeyInput helper around the enabled() gate and the nested-node call site. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[preValidateCredentialInputs] --> B{Main block inputs present?}
B -- yes --> C[getBlock type]
C --> D[collectHostedApiKeyInput - LLM model path]
C --> E{edit op and workflowState?}
E -- yes --> F[Merge existing subBlock values into toolParams]
E -- no --> G[toolParams = op.params.inputs]
F --> H[collectHostedToolApiKeyInput]
G --> H
B -- also --> Y[nestedNodes present?]
Y -- yes --> Z[for each child block]
Z --> AA[collectHostedToolApiKeyInput - childInputs only, no state merge]
H --> I{blockConfig.tools exists?}
I -- no --> J[return - skip]
I -- yes --> K{tools.config.tool selector?}
K -- yes --> L[try selector - catch return]
K -- no --> M[candidateToolIds = tools.access]
L --> N[candidateToolIds = single resolved tool]
N --> O[for each candidateToolId - getTool]
M --> O
O --> P{tool.hosting?}
P -- no --> Q[skip]
P -- yes --> R{hosting.enabled gate passes?}
R -- no --> Q
R -- yes --> S[managedFieldIds add apiKeyParam]
S --> T{field is non-empty string?}
T -- no --> Q
T -- yes --> U[push to hostedApiKeyInputs]
U --> V[structuredClone operations]
AA --> V
V --> W[Strip managed fields from filteredOperations]
W --> X[Push validation errors with stripMessage]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[preValidateCredentialInputs] --> B{Main block inputs present?}
B -- yes --> C[getBlock type]
C --> D[collectHostedApiKeyInput - LLM model path]
C --> E{edit op and workflowState?}
E -- yes --> F[Merge existing subBlock values into toolParams]
E -- no --> G[toolParams = op.params.inputs]
F --> H[collectHostedToolApiKeyInput]
G --> H
B -- also --> Y[nestedNodes present?]
Y -- yes --> Z[for each child block]
Z --> AA[collectHostedToolApiKeyInput - childInputs only, no state merge]
H --> I{blockConfig.tools exists?}
I -- no --> J[return - skip]
I -- yes --> K{tools.config.tool selector?}
K -- yes --> L[try selector - catch return]
K -- no --> M[candidateToolIds = tools.access]
L --> N[candidateToolIds = single resolved tool]
N --> O[for each candidateToolId - getTool]
M --> O
O --> P{tool.hosting?}
P -- no --> Q[skip]
P -- yes --> R{hosting.enabled gate passes?}
R -- no --> Q
R -- yes --> S[managedFieldIds add apiKeyParam]
S --> T{field is non-empty string?}
T -- no --> Q
T -- yes --> U[push to hostedApiKeyInputs]
U --> V[structuredClone operations]
AA --> V
V --> W[Strip managed fields from filteredOperations]
W --> X[Push validation errors with stripMessage]
|
1 participant
Summary
apiKey: "{{FAL_API_KEY}}"(and similar) onto hosted-tool blocks like the Fal video/image generators. On hosted Sim this both disabled hosted-key injection (a non-empty inline key short-circuitsinjectHostedKeyIfNeeded) and misled the agent into telling users they must bring their own key.edit_workflow'spreValidateCredentialInputsalready stripped+warned for hosted LLM models (getHostedModels()), but that path only covers Agent/Router blocks — tool-hosted blocks slipped through.tool.hosting— the same canonical signalinjectHostedKeyIfNeededuses at execution, so validator and execution agree by construction:tools.config.toolselector (soprovider: falai→video_falai),hosting.enabledgate (e.g.image_generatefalai-only),hosting.apiKeyParamfield (no hardcoded"apiKey"assumption),success: truepreserved).isHosted=false) and non-hosted providers (Runway/Veo/Luma/MiniMax) are untouched.Type of Change
Testing
vitestonvalidation.test.ts: 31 passed (added hosted-tool cases — provider routing, edit-op provider resolution from workflow state, nested-in-loop, non-apiKeyfield name,enabled-gate pass/fail, self-hosted preserved)bun run lint:check: cleanbun run check:api-validation:strict: passedtsc --noEmit: cleanChecklist