fix(copilot): strip hosted apiKey on type-less edit ops + guard hosting.enabled (#5220)

* fix(copilot): strip hosted apiKey on type-less edit ops + guard hosting.enabled

The hosted-apiKey strip in preValidateCredentialInputs was gated on op.params.type, but edit ops omit type (they carry only changed inputs). An apiKey-only edit on a hosted-tool block therefore skipped both the model and tool strip paths, so a copilot-authored key persisted and disabled hosted-key injection.

Resolve the block type from workflowState when the op omits it, so type-less edits run through the strip. Also guard tool.hosting.enabled() in try/catch like the tool selector. Add a regression test mirroring the observed failure (edit op with only apiKey, type+provider resolved from workflow state).

* improvement(copilot): consolidate hosted-apiKey workflowState reads, gate off-hosted

Quality cleanup (no behavior change): the main loop read workflowState.blocks[id] three times (block type, model fallback, toolParams merge). Collapse to one existingBlock lookup + one buildSubBlockValues; derive modelValue from the merged toolParams (drops the model-fallback ladder); gate the reconstruction on isHosted so buildSubBlockValues + spreads no longer run off-hosted or for blocks the collectors skip. Add an asRecord helper to cut repeated casts.

* fix(copilot): resolve hosted-key strip against same-batch state; strip on enabled throw

Addresses review on #5220:
- Batch-aware block state: a later type-less edit now sees type/provider changed by an earlier op in the same edit_workflow request (was reading the stale initial snapshot), so a key can't survive on a block an earlier op just made hosted.
- hosting.enabled() throwing now fails toward treating the key as managed (strip) instead of preserving it, since the hosting state is unknown on throw.

* fix(copilot): unify hosted-key strip resolution across top-level and nested blocks

Greptile flagged that the batch/snapshot state reconstruction was applied only to top-level blocks; nested loop/parallel children still used raw childInputs, so a same-batch provider/model change on a nested child followed by a type-less apiKey edit could leave the key. Route both paths through one collectForBlock helper keyed by the block's own id (incl. nested children), so they share batch accumulation + snapshot enrichment and can't drift. Test added for the nested same-batch case.

* fix(copilot): recurse nestedNodes so grandchild hosted keys are stripped

The collection loop and the strip/credential-removal loops only handled the first nestedNodes level, but the apply path processes nestedNodes recursively (loop/parallel children can themselves contain nestedNodes). A hosted key on a grandchild (e.g. loop-in-loop) survived. Collection now walks the nestedNodes tree recursively; the strip/removal loops locate a descendant's inputs at any depth via findNestedInputs. Test added for a two-level-deep grandchild.

* fix(copilot): decide hosted-key strip against final batch state (two-pass)

Forward-only accumulation missed reverse order (apiKey set in an early op, block made hosted by a later op) and let an earlier bogus/empty type poison snapshot fallback. Replace it with a two-pass approach: pass 1 folds every op (and nested descendant) into each block's FINAL effective type+values for the batch; pass 2 strips the managed fields each op sets, judged against that final state. Order-independent, any nesting depth, and empty/invalid types no longer block fallback (validType guard). Tests added for reverse order and bogus-type cases.

* fix(copilot): tool selector throw falls back to access tools (fail toward strip)

Symmetric with the enabled-throw fix: when tools.config.tool throws on partial params, scan all access tools instead of returning, so a hosted key can't slip through. Test added.

* fix(copilot): only fold registry-known types into final batch state

Pass 1 recorded any non-empty type into finalType, but apply skips type changes to unknown types (keeps the existing block). An unknown type on an earlier op could poison a later type-less apiKey edit. Only advance finalType to a getBlock-resolvable type so the fallback matches what apply persists. Test covers empty and unknown types.
This commit is contained in:
Theodore Li
2026-06-26 12:56:17 -07:00
committed by GitHub
parent 2fa3dd65bc
commit a33c173146
2 changed files with 389 additions and 111 deletions
@@ -114,6 +114,31 @@ const imageBlockConfig = {
tools: { access: ['image_generate'], config: { tool: () => 'image_generate' } },
}
// Tool whose hosting.enabled predicate throws — used to assert fail-toward-strip behavior.
const throwGateBlockConfig = {
type: 'throw_gate_block',
name: 'Throw Gate Block',
outputs: {},
subBlocks: [{ id: 'provider', type: 'dropdown' }],
tools: { access: ['throw_gate_tool'], config: { tool: () => 'throw_gate_tool' } },
}
// Block whose tool selector throws — should fall back to scanning access tools (video_falai).
const throwSelectorBlockConfig = {
type: 'throw_selector_block',
name: 'Throw Selector Block',
outputs: {},
subBlocks: [{ id: 'provider', type: 'dropdown' }],
tools: {
access: ['video_falai'],
config: {
tool: () => {
throw new Error('selector boom')
},
},
},
}
// Tool registry stand-in for the hosted-tool tests.
const toolsByIdMock: Record<string, unknown> = {
video_falai: { id: 'video_falai', hosting: { apiKeyParam: 'apiKey' } },
@@ -126,6 +151,15 @@ const toolsByIdMock: Record<string, unknown> = {
enabled: (p: Record<string, unknown>) => p.provider === 'falai',
},
},
throw_gate_tool: {
id: 'throw_gate_tool',
hosting: {
apiKeyParam: 'apiKey',
enabled: () => {
throw new Error('boom')
},
},
},
}
vi.mock('@/blocks/registry', () => ({
@@ -150,7 +184,11 @@ vi.mock('@/blocks/registry', () => ({
? customKeyBlockConfig
: type === 'image_generator_v2'
? imageBlockConfig
: undefined,
: type === 'throw_gate_block'
? throwGateBlockConfig
: type === 'throw_selector_block'
? throwSelectorBlockConfig
: undefined,
}))
vi.mock('@/blocks/utils', () => ({
@@ -469,6 +507,31 @@ describe('preValidateCredentialInputs (hosted-tool blocks)', () => {
expect(result.errors).toHaveLength(1)
})
it('strips apiKey on a type-less edit op, resolving block type + provider from workflow state', async () => {
// Mirrors the real failure: agent edits only { apiKey } with no `type` restated.
const operations = [
{
operation_type: 'edit' as const,
block_id: 'video-1',
params: { inputs: { apiKey: 'test-api-key-12345' } },
},
]
const workflowState = {
blocks: {
'video-1': {
type: 'video_generator_v3',
subBlocks: { provider: { value: 'falai' } },
},
},
}
const result = await preValidateCredentialInputs(operations, ctx, workflowState)
expect(result.filteredOperations[0]?.params?.inputs?.apiKey).toBeUndefined()
expect(result.errors).toHaveLength(1)
expect(result.errors[0]).toMatchObject({ blockId: 'video-1', field: 'apiKey' })
})
it('strips apiKey on a hosted-tool block nested inside a loop', async () => {
const operations = [
{
@@ -516,6 +579,198 @@ describe('preValidateCredentialInputs (hosted-tool blocks)', () => {
expect(result.errors[0]).toMatchObject({ blockId: 'custom-1', field: 'serviceKey' })
})
it('strips apiKey on a grandchild block nested two levels deep (loop in loop)', async () => {
const operations = [
{
operation_type: 'add' as const,
block_id: 'outer-loop',
params: {
type: 'loop',
inputs: {},
nestedNodes: {
'inner-loop': {
type: 'loop',
inputs: {},
nestedNodes: {
'video-child': {
type: 'video_generator_v3',
inputs: { provider: 'falai', apiKey: '{{FAL_API_KEY}}' },
},
},
},
},
},
},
]
const result = await preValidateCredentialInputs(operations, ctx)
const innerInputs = (
(result.filteredOperations[0]?.params?.nestedNodes as Record<string, any>)?.['inner-loop']
?.nestedNodes as Record<string, { inputs?: Record<string, unknown> }>
)?.['video-child']?.inputs
expect(innerInputs?.apiKey).toBeUndefined()
expect(result.errors).toHaveLength(1)
expect(result.errors[0]).toMatchObject({ blockId: 'video-child', field: 'apiKey' })
})
it('uses same-batch state for nested children (provider set earlier, apiKey set later)', async () => {
const operations = [
{
operation_type: 'add' as const,
block_id: 'loop-1',
params: {
type: 'loop',
inputs: {},
nestedNodes: {
'video-child': { type: 'video_generator_v3', inputs: { provider: 'falai' } },
},
},
},
{
operation_type: 'edit' as const,
block_id: 'loop-1',
params: {
nestedNodes: {
'video-child': { type: 'video_generator_v3', inputs: { apiKey: 'test-key' } },
},
},
},
]
const result = await preValidateCredentialInputs(operations, ctx)
const nested = result.filteredOperations[1]?.params?.nestedNodes as
| Record<string, { inputs?: Record<string, unknown> }>
| undefined
expect(nested?.['video-child']?.inputs?.apiKey).toBeUndefined()
expect(result.errors).toHaveLength(1)
expect(result.errors[0]).toMatchObject({ blockId: 'video-child', field: 'apiKey' })
})
it('strips a key set before a later op makes the block hosted (reverse batch order)', async () => {
// op1 sets apiKey while the block is still non-hosted (runway); op2 later flips it to falai.
// Deciding against final state must still strip op1's key.
const operations = [
{
operation_type: 'edit' as const,
block_id: 'video-1',
params: { inputs: { apiKey: '{{FAL_API_KEY}}' } },
},
{
operation_type: 'edit' as const,
block_id: 'video-1',
params: { inputs: { provider: 'falai' } },
},
]
const workflowState = {
blocks: {
'video-1': { type: 'video_generator_v3', subBlocks: { provider: { value: 'runway' } } },
},
}
const result = await preValidateCredentialInputs(operations, ctx, workflowState)
expect(result.filteredOperations[0]?.params?.inputs?.apiKey).toBeUndefined()
expect(result.errors).toHaveLength(1)
expect(result.errors[0]).toMatchObject({ blockId: 'video-1', field: 'apiKey' })
})
it.each([{ type: '' }, { type: 'totally_unknown_type' }])(
'does not let an invalid type (%o) on an earlier op block stripping on a later edit',
async ({ type }) => {
const operations = [
{
operation_type: 'edit' as const,
block_id: 'video-1',
params: { type, inputs: { prompt: 'x' } },
},
{
operation_type: 'edit' as const,
block_id: 'video-1',
params: { inputs: { apiKey: '{{FAL_API_KEY}}' } },
},
]
const workflowState = {
blocks: {
'video-1': { type: 'video_generator_v3', subBlocks: { provider: { value: 'falai' } } },
},
}
const result = await preValidateCredentialInputs(operations, ctx, workflowState)
expect(result.filteredOperations[1]?.params?.inputs?.apiKey).toBeUndefined()
expect(result.errors).toHaveLength(1)
}
)
it('uses same-batch state: a type-less apiKey edit after an earlier op makes the block hosted', async () => {
// op1 switches provider to falai (hosted); op2 (type-less) sets apiKey. op2 must see op1's
// provider, not the stale snapshot (runway), and strip the key.
const operations = [
{
operation_type: 'edit' as const,
block_id: 'video-1',
params: { inputs: { provider: 'falai' } },
},
{
operation_type: 'edit' as const,
block_id: 'video-1',
params: { inputs: { apiKey: 'test-api-key-12345' } },
},
]
const workflowState = {
blocks: {
'video-1': {
type: 'video_generator_v3',
subBlocks: { provider: { value: 'runway' } },
},
},
}
const result = await preValidateCredentialInputs(operations, ctx, workflowState)
expect(result.filteredOperations[1]?.params?.inputs?.apiKey).toBeUndefined()
expect(result.errors).toHaveLength(1)
expect(result.errors[0]).toMatchObject({ blockId: 'video-1', field: 'apiKey' })
})
it('strips apiKey when the tool selector throws (falls back to access tools)', async () => {
const operations = [
{
operation_type: 'add' as const,
block_id: 'sel-1',
params: {
type: 'throw_selector_block',
inputs: { provider: 'falai', apiKey: 'user-key' },
},
},
]
const result = await preValidateCredentialInputs(operations, ctx)
expect(result.filteredOperations[0]?.params?.inputs?.apiKey).toBeUndefined()
expect(result.errors).toHaveLength(1)
})
it('strips apiKey when a tool hosting enabled predicate throws (fail toward stripping)', async () => {
const operations = [
{
operation_type: 'add' as const,
block_id: 'gate-1',
params: {
type: 'throw_gate_block',
inputs: { provider: 'whatever', apiKey: 'user-key' },
},
},
]
const result = await preValidateCredentialInputs(operations, ctx)
expect(result.filteredOperations[0]?.params?.inputs?.apiKey).toBeUndefined()
expect(result.errors).toHaveLength(1)
})
it('preserves apiKey on self-hosted deployments (isHosted false)', async () => {
mockEnvFlags.isHosted = false
const operations = [
@@ -1332,24 +1332,37 @@ export async function preValidateCredentialInputs(
if (!isHosted || !blockConfig?.tools) return
// Resolve which tool(s) the current inputs select. With a selector there is exactly one active
// tool; without one, every accessible tool is a candidate.
// tool; without one (or if the selector throws on partial params), every accessible tool is a
// candidate — failing toward considering all hosted params so a key can't slip through.
const accessToolIds = blockConfig.tools.access ?? []
let candidateToolIds: string[]
const toolSelector = blockConfig.tools.config?.tool
if (toolSelector) {
try {
candidateToolIds = [toolSelector(toolParams)]
} catch {
return
candidateToolIds = accessToolIds
}
} else {
candidateToolIds = blockConfig.tools.access ?? []
candidateToolIds = accessToolIds
}
const managedFieldIds = new Set<string>()
for (const toolId of candidateToolIds) {
const tool = getTool(toolId)
if (!tool?.hosting) continue
if (tool.hosting.enabled && !tool.hosting.enabled(toolParams)) continue
// The enabled predicate is tool-defined; guard it so a throw can't break edit_workflow. On
// a throw the hosting state is unknown, so fail toward treating the key as managed (strip)
// rather than preserving a key that may actually be platform-managed.
if (tool.hosting.enabled) {
let isManaged: boolean
try {
isManaged = tool.hosting.enabled(toolParams)
} catch {
isManaged = true
}
if (!isManaged) continue
}
managedFieldIds.add(tool.hosting.apiKeyParam)
}
@@ -1377,113 +1390,131 @@ export async function preValidateCredentialInputs(
}
}
operations.forEach((op, opIndex) => {
// Process main block inputs
if (op.params?.inputs && op.params?.type) {
const blockConfig = getBlock(op.params.type)
if (blockConfig) {
// Collect credentials from main block
collectCredentialInputs(
blockConfig,
op.params.inputs as Record<string, unknown>,
const asRecord = (value: unknown): Record<string, unknown> | undefined =>
value && typeof value === 'object' ? (value as Record<string, unknown>) : undefined
const snapshotBlock = (blockId: string) => asRecord(asRecord(workflowState?.blocks)?.[blockId])
// nestedNodes can nest recursively (a loop/parallel child can itself contain nestedNodes), and
// the apply path processes them recursively — so find a descendant's inputs at any depth.
const findNestedInputs = (
nodes: unknown,
targetId: string
): Record<string, unknown> | undefined => {
const map = asRecord(nodes)
if (!map) return undefined
for (const [id, node] of Object.entries(map)) {
if (id === targetId) return asRecord(asRecord(node)?.inputs)
const found = findNestedInputs(asRecord(node)?.nestedNodes, targetId)
if (found) return found
}
return undefined
}
const validType = (type: unknown): string | undefined =>
typeof type === 'string' && type.trim() ? type : undefined
// Visit every block an op touches — its main block and each nestedNodes descendant (recursively,
// since loop/parallel children can themselves contain nestedNodes) — keyed by the block's own id.
const visitOpBlocks = (
op: EditWorkflowOperation,
opIndex: number,
visit: (b: {
opIndex: number
stateKey: string
reportBlockId: string
rawType: string | undefined
inputs: Record<string, unknown> | undefined
nestedBlockId?: string
}) => void
) => {
visit({
opIndex,
stateKey: op.block_id,
reportBlockId: op.block_id,
rawType: op.params?.type as string | undefined,
inputs: asRecord(op.params?.inputs),
})
const walk = (nodes: unknown, parentBlockId: string) => {
const map = asRecord(nodes)
if (!map) return
for (const [childId, childBlock] of Object.entries(map)) {
const child = asRecord(childBlock)
visit({
opIndex,
op.block_id,
op.params.type
)
// Check for apiKey inputs on hosted models
let modelValue = (op.params.inputs as Record<string, unknown>).model as string | undefined
// For edit operations, if model is not being changed, check existing block's model
if (
!modelValue &&
op.operation_type === 'edit' &&
(op.params.inputs as Record<string, unknown>).apiKey &&
workflowState
) {
const existingBlock = (workflowState.blocks as Record<string, unknown>)?.[op.block_id] as
| Record<string, unknown>
| undefined
const existingSubBlocks = existingBlock?.subBlocks as Record<string, unknown> | undefined
const existingModelSubBlock = existingSubBlocks?.model as
| Record<string, unknown>
| undefined
modelValue = existingModelSubBlock?.value as string | undefined
}
collectHostedApiKeyInput(
op.params.inputs as Record<string, unknown>,
modelValue,
opIndex,
op.block_id,
op.params.type
)
// The active tool depends on inputs (e.g. provider). On edit ops that don't restate
// provider, merge the existing block's subblock values so the tool selector and its
// `enabled` gate resolve correctly.
let toolParams = op.params.inputs as Record<string, unknown>
if (op.operation_type === 'edit' && workflowState) {
const existingBlock = (workflowState.blocks as Record<string, unknown>)?.[op.block_id] as
| Record<string, unknown>
| undefined
const existingSubBlocks = existingBlock?.subBlocks as
| Record<string, { value?: unknown } | null | undefined>
| undefined
if (existingSubBlocks) {
toolParams = { ...buildSubBlockValues(existingSubBlocks), ...toolParams }
}
}
collectHostedToolApiKeyInput(
blockConfig,
op.params.inputs as Record<string, unknown>,
toolParams,
opIndex,
op.block_id,
op.params.type
)
stateKey: childId,
reportBlockId: parentBlockId,
rawType: child?.type as string | undefined,
inputs: asRecord(child?.inputs),
nestedBlockId: childId,
})
walk(child?.nestedNodes, parentBlockId)
}
}
walk(op.params?.nestedNodes, op.block_id)
}
// Process nested nodes (blocks inside loop/parallel containers)
const nestedNodes = op.params?.nestedNodes as
| Record<string, Record<string, unknown>>
| undefined
if (nestedNodes) {
Object.entries(nestedNodes).forEach(([childId, childBlock]) => {
const childType = childBlock.type as string | undefined
const childInputs = childBlock.inputs as Record<string, unknown> | undefined
if (!childType || !childInputs) return
// Pass 1: fold every op into each block's FINAL effective state (type + values) for the batch,
// seeded from the snapshot. Deciding strips against the final state (not a forward prefix) makes
// order irrelevant — a key set before a later op makes the block hosted is still caught.
const finalType = new Map<string, string | undefined>()
const finalValues = new Map<string, Record<string, unknown>>()
for (const [opIndex, op] of operations.entries()) {
visitOpBlocks(op, opIndex, ({ stateKey, rawType, inputs }) => {
const priorType = finalType.has(stateKey)
? finalType.get(stateKey)
: (snapshotBlock(stateKey)?.type as string | undefined)
// Only advance the type to one apply will honor: a registry-known type. An unknown type
// (apply skips the change and keeps the existing block) must not poison the fallback, or a
// later type-less apiKey edit would be judged against a block config that never applies.
const candidate = validType(rawType)
const resolved = (candidate && getBlock(candidate) ? candidate : undefined) ?? priorType
if (resolved) finalType.set(stateKey, resolved)
if (isHosted) {
const priorValues =
finalValues.get(stateKey) ??
buildSubBlockValues(
(snapshotBlock(stateKey)?.subBlocks as Record<string, { value?: unknown }>) ?? {}
)
finalValues.set(stateKey, { ...priorValues, ...(inputs ?? {}) })
}
})
}
const childBlockConfig = getBlock(childType)
if (!childBlockConfig) return
// Pass 2: for each op, strip the managed fields it actually sets, judged against the block's
// final state. Both top-level and nested blocks route through the same visit so they can't drift.
for (const [opIndex, op] of operations.entries()) {
visitOpBlocks(op, opIndex, ({ stateKey, reportBlockId, inputs, nestedBlockId }) => {
const blockType = finalType.get(stateKey)
if (!inputs || !blockType) return
const blockConfig = getBlock(blockType)
if (!blockConfig) return
// Collect credentials from nested block
collectCredentialInputs(
childBlockConfig,
childInputs,
collectCredentialInputs(blockConfig, inputs, opIndex, reportBlockId, blockType, nestedBlockId)
// Hosted collectors no-op off hosted Sim, so only resolve the effective state when it matters.
if (isHosted) {
const toolParams = finalValues.get(stateKey) ?? inputs
const modelValue = toolParams.model as string | undefined
collectHostedApiKeyInput(
inputs,
modelValue,
opIndex,
op.block_id,
childType,
childId
reportBlockId,
blockType,
nestedBlockId
)
// Check for apiKey inputs on hosted models in nested block
const modelValue = childInputs.model as string | undefined
collectHostedApiKeyInput(childInputs, modelValue, opIndex, op.block_id, childType, childId)
collectHostedToolApiKeyInput(
childBlockConfig,
childInputs,
childInputs,
blockConfig,
inputs,
toolParams,
opIndex,
op.block_id,
childType,
childId
reportBlockId,
blockType,
nestedBlockId
)
})
}
})
}
})
}
const hasCredentialsToValidate = credentialInputs.length > 0
const hasHostedApiKeysToFilter = hostedApiKeyInputs.length > 0
@@ -1510,11 +1541,7 @@ export async function preValidateCredentialInputs(
// Handle nested block apiKey filtering
if (apiKeyInput.nestedBlockId) {
const nestedNodes = op.params?.nestedNodes as
| Record<string, Record<string, unknown>>
| undefined
const nestedBlock = nestedNodes?.[apiKeyInput.nestedBlockId]
const nestedInputs = nestedBlock?.inputs as Record<string, unknown> | undefined
const nestedInputs = findNestedInputs(op.params?.nestedNodes, apiKeyInput.nestedBlockId)
if (nestedInputs?.[field]) {
nestedInputs[field] = undefined
logger.debug('Filtered platform-managed apiKey in nested block', {
@@ -1573,11 +1600,7 @@ export async function preValidateCredentialInputs(
// Handle nested block credential removal
if (credInput.nestedBlockId) {
const nestedNodes = op.params?.nestedNodes as
| Record<string, Record<string, unknown>>
| undefined
const nestedBlock = nestedNodes?.[credInput.nestedBlockId]
const nestedInputs = nestedBlock?.inputs as Record<string, unknown> | undefined
const nestedInputs = findNestedInputs(op.params?.nestedNodes, credInput.nestedBlockId)
if (nestedInputs?.[credInput.fieldName]) {
delete nestedInputs[credInput.fieldName]
logger.info('Removed invalid credential from nested block', {