fix(executor): copy the env map at the workflow-tool boundary - #6618
Conversation
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.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryLow Risk Overview Previously Reviewed by Cursor Bugbot for commit a320f0b. Configure here. |
Greptile SummaryThis PR hardens the workflow-tool execution boundary by shallow-copying the invoking run’s environment-variable map before forwarding it to a child workflow.
Confidence Score: 5/5The PR appears safe to merge because the copy preserves the established environment-variable contract while removing cross-run reference aliasing. The production environment map is normalized to a plain string record, the child accepts the same record shape without relying on object identity, and the regression test covers the intended isolation behavior.
|
| Filename | Overview |
|---|---|
| apps/sim/tools/index.ts | Replaces direct environment-map forwarding with a shallow copy, preserving the string-record contents while isolating parent and child references. |
| apps/sim/tools/index.test.ts | Adds focused coverage confirming value forwarding, distinct object identity, and parent isolation from child-side mutations. |
Reviews (1): Last reviewed commit: "fix(executor): copy the env map at the w..." | Re-trigger Greptile
Summary
Follow-up hardening to #6611, which began forwarding the invoking run's environment variables into a workflow executed as an Agent tool.
A full runtime audit of #6611 confirmed that nothing in the child execution path writes through
ctx.environmentVariablestoday — so this is not a live defect. It closes the one structural gap that audit surfaced.Why
tools/index.tswas the only consumer that handed the parent's live env map across an execution boundary by reference, and it hands it to the longest-lived consumer there is — the child holds that object for its entire run, acrossWorkflowBlockHandler.executeCore's prologue.Every other consumer already copies before handing the map on:
agent-handler.ts:2496,:2576normalizeStringRecord(ctx.environmentVariables)function-handler.ts:87normalizeStringRecord(...)condition-handler.ts:52normalizeStringRecord(...)providers/utils.ts:1629normalizeStringRecord(...)tools/index.ts(workflow tool)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, and exactly the kind of thing that is invisible in review.
Why a shallow spread is exact
The value is typed
Record<string, string>, and the sub-Executor already re-copies it throughnormalizeStringRecordatexecutor.ts:73, so the child receives a byte-identical map either way. The spread also subsumes the previous?? {}— spreadingundefinedyields{}.Type of Change
Testing
New test mutates the forwarded map and asserts the parent context is unchanged; it fails without the spread. Full
executor/+tools/index.test.tssuite: 2096 passed. Type-check and Biome clean.Checklist