fix(copilot): strip hosted apiKey on type-less edit ops + guard hosting.enabled by TheodoreSpeaks · Pull Request #5220 · simstudioai/sim

TheodoreSpeaks · 2026-06-26T00:50:36Z

Summary

Follow-up to #5217. That PR stripped copilot-authored platform-managed apiKey on hosted-tool blocks, but only for ops that restate type — so a type-less edit op slipped through.

Verified live on staging via [trace 84d38867…]: prompted with "directly set apiKey for the makeVideo block", the agent issued { params: { inputs: { apiKey: "test-api-key-12345" } }, operation_type: "edit" } (no type). preValidateCredentialInputs gates the whole strip path (both the model and tool paths) on op.params.type, so the op was skipped and the key persisted — re-disabling hosted-key injection.

Resolve the block type from workflowState when the op omits type, so type-less edits run through the strip (also closes the same latent gap in the original hosted-LLM-model strip).
Guard tool.hosting.enabled() in try/catch, matching the existing guard on the tool selector, so a throwing predicate can't break edit_workflow.
Regression test mirroring the observed failure (edit op with only apiKey; type + provider resolved from workflow state).

Known remaining gap (lower-likelihood, not addressed here): nested edit ops inside a loop/parallel don't get workflow-state enrichment, so a nested type/provider-less edit could still slip through.

Type of Change

Bug fix

Testing

vitest on validation.test.ts: 57 passed (added type-less-edit regression case)
bun run lint:check: clean
bun run check:api-validation:strict: passed
tsc --noEmit: clean
Runtime wiring confirmed: edit-workflow/index.ts passes workflowState to preValidateCredentialInputs

Checklist

Code follows project style guidelines
Self-reviewed my changes
Tests added/updated and passing
No new warnings introduced
I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

…ng.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).

vercel · 2026-06-26T00:50:42Z

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment

Project	Deployment	Actions	Updated (UTC)
docs	Skipped		Jun 26, 2026 2:01am

TheodoreSpeaks · 2026-06-26T00:50:45Z

@greptile review

cursor · 2026-06-26T00:50:46Z

PR Summary

Medium Risk
Changes credential/apiKey filtering on the copilot edit path on hosted deployments; incorrect final-state logic could strip legitimate user keys or miss platform-managed ones, but behavior is heavily regression-tested.

Overview
Hardens copilot edit_workflow pre-validation so platform-managed keys on hosted Sim are stripped reliably, including the staging case where the agent issued a type-less edit with only apiKey.

preValidateCredentialInputs is refactored into two passes: first it folds the whole operation batch (plus workflowState) into each block’s final type and input values, then it decides what to strip per op against that final state so order no longer matters (e.g. apiKey before a later op switches provider to a hosted tool). Block type is taken from workflowState when the op omits type, and only registry-known types advance the effective type so bogus type values don’t block later strips.

Nested loop/parallel children are handled recursively via a shared visitor and findNestedInputs, including multi-level nesting and same-batch nested edits.

Fail-safe hosting resolution: if the block’s tool selector throws, candidate tools fall back to the block’s access list; if tool.hosting.enabled throws, the key is treated as managed and stripped. Regression tests cover type-less edits, batch ordering, invalid types, deep nesting, and both throw paths.

^{Reviewed by Cursor Bugbot for commit 84ddf57. Bugbot is set up for automated code reviews on this repo. Configure here.}

greptile-apps · 2026-06-26T00:53:47Z

Greptile Summary

This PR tightens hosted credential filtering in edit_workflow. The main changes are:

Resolves type-less edits from workflow state.
Builds final effective block state across a batch before stripping keys.
Recursively visits nested workflow blocks.
Strips hosted keys when tool selectors or hosting gates throw.
Adds tests for type-less edits, nested blocks, reverse-order batches, and throwing predicates.

Confidence Score: 4/5

This is close, but one nested credential path should be fixed before merging.

Top-level type-less edits and same-batch hosted state are now handled.
Existing nested children still cannot recover their snapshot type when an edit omits type.
That path can leave a platform-managed key in the saved workflow.

apps/sim/lib/copilot/tools/server/workflow/edit-workflow/validation.ts

Security Review

A hosted platform-managed API key can still survive a type-less edit of an existing nested loop or parallel child.

Important Files Changed

Filename	Overview
apps/sim/lib/copilot/tools/server/workflow/edit-workflow/validation.ts	Refactors credential pre-validation into a final-state pass with recursive nested traversal, but existing nested children still miss snapshot state on type-less edits.
apps/sim/lib/copilot/tools/server/workflow/edit-workflow/validation.test.ts	Adds tests for hosted credential stripping across type-less edits, nested blocks, batch ordering, and throwing hosted-tool predicates.

_{Reviews (8): Last reviewed commit: "fix(copilot): only fold registry-known t..." | Re-trigger Greptile}

greptile-apps · 2026-06-26T00:53:53Z

Greptile Summary

This PR tightens hosted API-key validation for copilot workflow edits. The main changes are:

Resolve omitted edit type values from workflow state.
Reuse the resolved block type for hosted model and tool key stripping.
Guard hosted-tool enabled checks from throwing.
Add a regression test for type-less hosted-tool edits.

Confidence Score: 4/5

The same-request type-change path needs a fix before merging.

A later type-less edit can be validated against the old workflow-state block type.
That can leave a hosted apiKey override in place for the new block type.
The direct type-less edit regression is otherwise covered by the new test.

apps/sim/lib/copilot/tools/server/workflow/edit-workflow/validation.ts

Security Review

A same-request type change can still let a later type-less apiKey edit validate against the old block type, which can leave a hosted credential override in place.

Important Files Changed

Filename	Overview
apps/sim/lib/copilot/tools/server/workflow/edit-workflow/validation.ts	Adds workflow-state type fallback and hosted-tool guard logic, but the fallback can use stale state when multiple edits target the same block in one request.
apps/sim/lib/copilot/tools/server/workflow/edit-workflow/validation.test.ts	Adds coverage for a type-less edit that sets a hosted-tool `apiKey` while the existing workflow state provides the block type and provider.

_{Reviews (1): Last reviewed commit: "fix(copilot): strip hosted apiKey on typ..." | Re-trigger Greptile}

…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.

…p 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.

TheodoreSpeaks · 2026-06-26T01:05:08Z

Addressed the P1s in 71683d0:

Same-batch stale state: block type + subblock values are now tracked across ops in the request, so a later type-less apiKey edit sees provider/type changed by an earlier op in the same batch (no longer the initial snapshot). Test added.
Throwing enabled predicate: now fails toward treating the key as managed (strip) instead of preserving it. Test added.

@greptile review

…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.

TheodoreSpeaks · 2026-06-26T01:17:57Z

Re-review follow-ups:

Nested state skipped (validation.ts:1427) — fixed in 8ae1f80. Top-level and nested blocks now route through one collectForBlock helper keyed by the block's own id (including nested children), so nested children get the same batch + snapshot resolution. Added a test for the nested same-batch case (provider set in op1, apiKey in op2 → stripped).
Throwing predicate preserves keys (validation.ts:1361) — this one's a false positive on the re-flag: the catch sets isManaged = true, so a throwing enabled now strips the key (fails toward managed), it doesn't preserve it. Covered by the test strips apiKey when a tool hosting enabled predicate throws.

@greptile review

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.

TheodoreSpeaks · 2026-06-26T01:27:59Z

Nested grandchildren skip validation — fixed in $(git rev-parse --short HEAD). Collection now walks the nestedNodes tree recursively, and the strip/credential-removal loops locate a descendant's inputs at any depth (findNestedInputs), matching the recursive apply path. Added a loop-in-loop grandchild test.

@greptile review

…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.

TheodoreSpeaks · 2026-06-26T01:39:58Z

Both addressed in 2eb8416 by switching to a two-pass model:

Reverse batch order (L1497): strips are now decided against each block's final effective batch state (pass 1 folds all ops → final type+values; pass 2 strips fields each op sets against that). Order no longer matters — a key set before a later op makes the block hosted is caught. Test added.
Invalid batch type blocks stripping (L1440): empty/whitespace type is treated as absent (validType) and never cached, so it can't override snapshot fallback on a later edit. Test added.

This also generalizes the earlier same-batch/nested/grandchild fixes into one order- and depth-independent pass.

@greptile review

…ward 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.

TheodoreSpeaks · 2026-06-26T01:50:34Z

Selector throws open (L1363) — fixed in 1a01720: a throwing tools.config.tool now falls back to scanning all access tools (symmetric with the enabled-throw handling) instead of returning, so the key is still stripped. Test added.
Final batch type skips stripping (L1484) — intentionally not changing this. Deciding against the block's final batch type is the correct basis: it's what the apply step persists, and it's exactly what the reverse-order P1 (L1497) required. Using the author-time type instead would re-open that bug and would wrongly strip a user's legitimate key when a later op makes the final block non-hosted. The only residual case is an op that sets an invalid type — that's rejected by the apply/type validation layer, not something the credential strip should compensate for (empty/whitespace types are already ignored via validType). Happy to add a union-of-all-batch-states strip if you want maximum conservatism, but it would over-strip legit non-hosted keys.

@greptile review

cursor

Cursor Bugbot has reviewed your changes and found 1 potential issue.

There are 2 total unresolved issues (including 1 from previous review).

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit 1a01720. Configure here.}

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.

TheodoreSpeaks · 2026-06-26T02:01:23Z

Rejected edit type poisons strip (L1467) — fixed in 84ddf57: pass 1 now only advances finalType to a registry-known type (getBlock-resolvable). An unknown type (which apply skips, keeping the existing block) no longer poisons a later type-less apiKey edit. Test covers empty and unknown types.
Nested snapshot missing (L1461) — false positive. Nested loop/parallel children are stored flat in workflowState.blocks[childId] (keyed by their own id), not under the parent's nestedNodes — see operations.ts processNestedNodesForParent: modifiedState.blocks[childId] = childBlockState. nestedNodes is only the copilot op authoring shape; the persisted snapshot is flat. So snapshotBlock(childId) does recover an existing nested child's type/provider. No change needed.

@greptile review

greptile-apps · 2026-06-26T02:03:52Z

-          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])


Nested snapshot missing This lookup only checks workflowState.blocks[blockId], but existing nested children live under their parent nestedNodes. When a later edit touches an existing loop or parallel child with only inputs.apiKey and does not restate the child type, the final-state pass cannot recover the child type, so the hosted-key collectors are skipped and the platform-managed key can still be persisted.

TheodoreSpeaks · 2026-06-26T02:07:48Z

Re L1395 (nested snapshot missing) — this is the same claim as the earlier L1461 thread and it's a false positive. Persisted nested children are stored flat in workflowState.blocks[childId], not under the parent's nestedNodes. Evidence: edit-workflow/operations.ts processNestedNodesForParent does modifiedState.blocks[childId] = childBlockState (and recurses for deeper children). nestedNodes is only the copilot op authoring shape; once applied, every child — at any depth — is a top-level blocks entry keyed by its own id with a parentId ref. So snapshotBlock(childId) correctly recovers an existing nested child's type/provider, and the final-state pass resolves it. No change needed.

cursor Bot reviewed Jun 26, 2026

View reviewed changes

Comment thread apps/sim/lib/copilot/tools/server/workflow/edit-workflow/validation.ts