fix(executor): restore delegated workflow execution - #6539
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryHigh Risk Overview Workflow authorization now lets an active executor delegation read any workflow in the same workspace, not only the workflow id embedded in the token—aligned with loading child workflow definitions. Agent / provider tool path gains safer handling when models echo Executor-delegation protected tools mint tokens from Reviewed by Cursor Bugbot for commit c7ef54c. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
@cursor review |
Greptile SummaryThe PR restores delegated execution across nested workflows and agent tools while preserving trusted execution identity and secret provenance.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| apps/sim/providers/utils.ts | Safely resolves model-exposed environment references and fixes the previously reported merge-result typing issue. |
| apps/sim/executor/handlers/workflow/workflow-handler.ts | Preserves canonical delegation identity for regular children and establishes source-owned identity for custom-block executions. |
| apps/sim/tools/index.ts | Uses trusted execution scope for protected tools and overwrites model-supplied custom-block context fields. |
| apps/sim/executor/utils/resolved-secret-trace-registry.ts | Adds scoped placeholder rebinding with collision detection and per-tool provenance tracking. |
| apps/sim/lib/workflows/application/authorization.ts | Permits delegated workflow reads across workflows while retaining workspace and canonical-origin checks. |
Reviews (3): Last reviewed commit: "fix(providers): keep tool parameters typ..." | Re-trigger Greptile
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit c7ef54c. Configure here.
…policy (#6611) A workflow attached as an Agent (or Pi) tool ran its entire child execution with an empty environment-variable map and no block-output redaction policy. Mechanism. `tools/index.ts` short-circuits `workflow_executor` into `runWorkflowTool`, which builds its synthetic parent `ExecutionContext` with `buildCustomBlockExecutionContext`. That builder was written for the custom-block (deploy-as-block) path and hardcoded `environmentVariables: {}` — safe there only because `WorkflowBlockHandler.executeCore` re-derives the publisher's env inside `if (isCustomBlock)`. The workflow-tool path's synthetic block carries `metadata.id: 'workflow_input'`, so `isCustomBlock` is false, the re-derivation is skipped, and `{}` flows through `childEnvVarValues` into the sub-Executor. `DAGExecutor` has no fallback and `EnvResolver` returns the raw reference on a miss, so a child block field of `Bearer {{MY_API_KEY}}` was transmitted to the third party verbatim and 401'd — silently, with the variable name disclosed. The same builder never set `piiBlockOutputRedaction`, so `block-executor`'s in-flight masking was disabled for every child block of orgs that had explicitly enabled that stage. Both landed as unnoticed side effects of #5273, whose stated goals were admission slots, log rows, cost roll-up and structured errors; #6539 later patched a third dropped field on the same context without noticing these two. Fix. Thread both values through the runner `options` bag — never `params._context`, which spreads model-reachable `contextParams._context` first and would let a model inject its own env map or disable redaction. `executeTool` reads them off the trusted `executionContext`, which also covers the Pi block, whose tool loop calls `executeTool` with `executionContext: ctx` on the identical path. `environmentVariables` is required rather than optional-with-a-default. Silent omission is precisely the failure mode here and in #6539; making it required turns the next caller's omission into a compile error. `runCustomBlockTool` now passes `{}` explicitly, so that path is unchanged at runtime. `piiBlockOutputRedaction` stays optional deliberately: `undefined` is its correct value for the many tenants with no policy, whereas `{}` for env is a wrong identity rather than a default. The builder's TSDoc states both halves of that asymmetry. Identity semantics — this restores function but does not restore main's identity. On main this tool was an HTTP hop into execution-core, which derived the env from the CHILD workflow's owner, so the child got the child owner's personal env plus the child workspace's env. Forwarding the caller's map gives the child the PARENT CALLER's personal env: a different identity, not a subset. That is the deliberate choice, because it is byte-identical to the long-standing canvas workflow block, it is bounded to one workspace by `assertChildWorkflowInWorkspace` on this branch, and it is the only variant consistent with the parent `resolvedSecretTraceRegistry` this path already forwards. The narrow case that worked on main and still will not: a same-workspace child owned by another member that relied on THAT member's personal environment variable. The `deployed_block_executor` call site deliberately gets neither value: custom blocks skip the same-workspace assert and run cross-workspace under the publisher's identity, so the consumer's env and redaction rules are the wrong tenant's. A test pins that so a later refactor cannot unify the branches silently. Tests. Three suites pin the fix itself (runner, builder, `executeTool` dispatch) and go red without it. A fourth case in `workflow-handler.test.ts` pins the last hop — `ctx.environmentVariables` -> `childEnvVarValues` -> the sub-Executor's `envVarValues`, plus `piiBlockOutputRedaction` — on the NON-custom branch. That hop is untouched staging code, so that case passes either way by construction; it exists so a future change to the branch that distinguishes the two paths cannot silently undo this fix downstream of the builder. Out of scope, deliberately: `enforceCredentialAccess` is dropped by the same synthetic context, but on main this path ran under an internal JWT with `useAuthenticatedUserAsActor === false`, so forwarding the parent's value would TIGHTEN behavior versus main and could break currently-working child runs mid-release. It needs its own deliberate change — and it now compounds with this one, since the child runs with the parent's decrypted env while credential-access enforcement stays off. The `input` redaction stage (masking the LLM-authored inputMapping) is also not restored — `ExecutionContext` has no field for it and the canvas workflow block never had it either. Re-enabling masking inside child runs is a live behavior change for affected tenants: `redactObjectStrings` runs with `onFailure: 'throw'`, so a child agent tool call that currently succeeds unmasked can now fail closed, which is main's semantic restored. This belongs in the release note.
Summary
Type of Change
Testing
Checklist