Uh oh!
There was an error while loading. Please reload this page.
fix(executor): give the workflow agent tool the caller's env and PII policy - #6611
Conversation
…policy
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.The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryMedium Risk Overview
Custom blocks stay excluded — they still get Reviewed by Cursor Bugbot for commit 321438a. Configure here. |
Greptile SummaryThis PR restores the invoking workflow’s environment variables and block-output PII policy when an agent or Pi block executes a same-workspace workflow tool.
Confidence Score: 5/5The PR appears safe to merge, with the changed workflow-tool path consistently forwarding trusted environment and redaction context while preserving cross-workspace custom-block isolation. The complete propagation chain reaches the child executor, same-workspace validation bounds the forwarded identity, model-reachable context cannot override the trusted values, and all required internal callers provide the new environment option.
|
| Filename | Overview |
|---|---|
| apps/sim/tools/index.ts | Forwards trusted environment variables and PII policy to the workflow runner while leaving deployed custom-block tenant isolation unchanged. |
| apps/sim/executor/handlers/workflow/custom-block-tool-runner.ts | Extends synthetic execution-context construction with a required environment map and optional block-output redaction policy. |
| apps/sim/executor/handlers/workflow/workflow-tool-runner.ts | Requires the invoking environment and carries both environment and redaction policy into non-custom child workflow execution. |
| apps/sim/executor/handlers/workflow/workflow-tool-runner.test.ts | Covers trusted environment propagation, PII-policy propagation, and rejection of model-smuggled environment values. |
| apps/sim/tools/index.test.ts | Verifies trusted-context forwarding and deliberate exclusion from deployed custom-block execution. |
Sequence Diagram
sequenceDiagram
participant Parent as Parent Executor
participant Tools as executeTool
participant Runner as runWorkflowTool
participant Handler as WorkflowBlockHandler
participant Child as Child Executor
Parent->>Tools: workflow_executor + trusted ExecutionContext
Tools->>Runner: env map, PII policy, trusted scope
Runner->>Handler: synthetic non-custom context
Handler->>Handler: assert child is in parent workspace
Handler->>Child: parent env map and PII policy
Child-->>Parent: redacted child result
Reviews (1): Last reviewed commit: "fix(executor): give the workflow agent t..." | Re-trigger Greptile
Uh oh!
There was an error while loading. Please reload this page.
Follow-up hardening to #6611, which began forwarding the invoking run's environment variables into a workflow run as an agent tool. A runtime audit of that change confirmed nothing today writes through `ctx.environmentVariables`, so this is not a live defect. But `tools/index.ts` was the only consumer handing the map across an execution boundary by reference, and it hands it to the longest-lived consumer there is: the child holds it for its entire run. `agent-handler`, `function-handler`, `condition-handler` and `providers/utils` all copy via `normalizeStringRecord` before handing the map anywhere. A future write through the child's reference would corrupt the parent's env and every later sibling tool call in the same agent turn — a cross-run bug with no local symptom. A shallow spread is exact here: the value is typed `Record<string, string>`, and the sub-Executor already re-copies it through `normalizeStringRecord` (`executor.ts:73`), so the child receives a byte-identical map either way. The spread also subsumes the previous `?? {}`, since spreading `undefined` yields `{}`. The test mutates the forwarded map and asserts the parent context is unchanged; it fails without the spread.
Summary
A workflow attached as an Agent tool (
workflow_executor) ran its entire child execution withenvironmentVariables: {}and no block-output PII redaction policy. The Pi block reaches the identical path (sim-tools.tscallsexecuteToolwithexecutionContext: ctx), so it was affected the same way.The failure was silent, and it leaked. With an empty env map the child's resolver cannot resolve
{{MY_API_KEY}}, soEnvResolverreturns the raw reference and the literal string{{MY_API_KEY}}is transmitted to the third-party API. The vendor 401s, the user sees an opaque auth error, and the variable name has already been handed to the vendor in anAuthorizationheader or request body. Nothing in Sim reports "environment variable missing".Separately, tenants who had explicitly enabled block-output PII redaction had it silently not applied to any child block in these runs.
Mechanism
buildCustomBlockExecutionContexthardcodedenvironmentVariables: {}when building the synthetic top-levelExecutionContext. That was safe for the branch it was written for:WorkflowBlockHandler.executeCorere-derives the publisher's env fromgetCustomBlockAuthorityinsideif (isCustomBlock), so the{}is provably overwritten before it is read.The workflow-as-agent-tool path's synthetic block carries
metadata.id: 'workflow_input'— it is not a custom block. It takes the non-custom branch, which keepsctx.environmentVariablesas-is, so the hardcoded{}flowed straight into the sub-Executor. Both fields landed as unnoticed side effects of #5273; #6539 later patched a third dropped field on the same context without noticing these two.Fix
Thread both values through the runner
optionsbag.executeToolreads them off the trustedexecutionContext, neverparams._context— that bag spreads model-reachablecontextParams._contextfirst, so sourcing from it would let a model inject its own env map or disable redaction. A test pins that a smuggled_context.environmentVariablesis ignored.Design decisions — please review these deliberately
environmentVariablesis required, not optional-with-a-default. This is the entire recurrence guarantee. An empty env map is a wrong identity, not an absent value, and silent omission is exactly how this shipped — twice. Making it optional shortens the diff and re-arms the footgun; a future caller that forgets it is now a compile error.piiBlockOutputRedactionstays optional. Hereundefinedgenuinely is the correct value — most tenants have no policy at all. The asymmetry is intentional and documented in the TSDoc precisely so a future reader does not "unify" it.deployed_block_executordeliberately receives 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 would be the wrong tenant's. A test pins this so a later refactor cannot silently unify the branches.Identity semantics — an honest note
This is not main's behavior, and it is not a strict subset of it. On main this tool was an HTTP hop into execution-core, which derived env from the child workflow's owner. This forwards the parent caller's map, matching the long-standing canvas workflow block (
workflow-handler.tskeepsctx.environmentVariableson the non-custom branch). It is a different identity, not a narrower one — chosen because it matches the canvas block byte-for-byte, is bounded to one workspace byassertChildWorkflowInWorkspace, and is the only variant consistent with the parentresolvedSecretTraceRegistrythis path already forwards.The narrow case that worked on main and still will not: a same-workspace child owned by another workspace member that relied on THAT member's personal environment variables.
Re-enabling block-output masking inside child runs means redaction now runs there with
onFailure: 'throw'(block-executor.tsaborts rather than feed unmasked data downstream). For tenants who enabled block-output PII redaction, a child agent-tool call that succeeds unmasked today can now fail closed. That restores main's intended semantic — the control was always supposed to apply — but affected users will feel it as new failures.Known gap, deliberately out of scope
tools/index.tscoalescesexecutionContext?.environmentVariables ?? {}becauseexecutionContextis optional on the options bag./api/providersbuilds a runtime context without one and accepts a caller-suppliedtoolsarray, so aworkflow_executor_<id>posted there would hit the same empty-map defect. No in-repo caller does this today, and closing it means changingexecuteTool's identity contract — its own change, not this one.enforceCredentialAccessis dropped by the same synthetic context and is likewise deferred: forwarding the parent's value would tighten behavior versus main and could break currently-working child runs mid-release.Type of Change
Testing
233 tests across 4 suites. Reverting the three source files turns exactly 5 red — including the model-smuggled-env-map test and the
deployed_block_executor-exclusion test. Type-check and Biome clean.Checklist