fix(sdk): resolve undefined name variable in updateEnvVar overload - #4494
fix(sdk): resolve undefined name variable in updateEnvVar overload#4494okxint wants to merge 2 commits into
Conversation
🦋 Changeset detectedLatest commit: 19c99f2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 26 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Hi @okxint, thanks for your interest in contributing! This project requires that pull request authors are vouched, and you are not in the list of vouched users. This PR will be closed automatically. See https://github.com/triggerdotdev/trigger.dev/blob/main/CONTRIBUTING.md for more details. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| $projectRef = projectRefOrName; | ||
| $slug = slugOrParams; | ||
| $name = name!; | ||
| $name = nameOrRequestOptions as string; |
There was a problem hiding this comment.
🔴 Updating an environment variable from inside a running task still targets the wrong project
When the four-argument form is used from inside a running task, the project identifier is taken from the slug argument instead of the project argument ($projectRef = slugOrParams at packages/trigger-sdk/src/v3/envvars.ts:308), so the update is sent to a non-existent project and fails.
Impact: Users calling the update helper with an explicit project and slug from within a task get a failed/incorrect update instead of the variable being changed.
Why the fix is incomplete: only the no-task-context branch was corrected
The implementation signature is (projectRefOrName, slugOrParams, nameOrRequestOptions, params, requestOptions). The PR fixed the else branch (no task context) at packages/trigger-sdk/src/v3/envvars.ts:339-342, which now correctly maps projectRefOrName -> $projectRef, slugOrParams -> $slug, nameOrRequestOptions -> $name.
But inside a task (taskContext.ctx truthy) the very same overload takes packages/trigger-sdk/src/v3/envvars.ts:307-319, where:
$projectRef = slugOrParams(the slug, not the project ref)$slug = slugOrParams ?? ...(the??is dead sinceslugOrParamsis a string here)$namefalls back totaskContext.ctx.environment.slugwhen the name is missing, which would silently update a variable named after the environment.
Compare with retrieve (packages/trigger-sdk/src/v3/envvars.ts:208-219) and del (packages/trigger-sdk/src/v3/envvars.ts:255-266) which correctly use projectRefOrName for $projectRef.
Prompt for agents
In packages/trigger-sdk/src/v3/envvars.ts, the `update` implementation resolves its arguments differently depending on whether `taskContext.ctx` exists. The PR fixed the no-context branch so that `$name` comes from `nameOrRequestOptions`, but the in-task branch (the `if (taskContext.ctx)` / `typeof slugOrParams === 'string'` path) still assigns `$projectRef = slugOrParams`, i.e. it uses the slug as the project reference, and it defaults `$name` to the environment slug when no name is supplied. For the `(projectRef, slug, name, params)` overload the mapping should be identical in both branches: `$projectRef = projectRefOrName`, `$slug = slugOrParams`, `$name = nameOrRequestOptions`. Consider unifying the argument resolution for the two branches (see how `retrieve` and `del` in the same file do it) so the in-task path is not broken.
Was this helpful? React with 👍 or 👎 to provide feedback.
| "@trigger.dev/sdk": patch | ||
| --- | ||
|
|
||
| Fix `updateEnvVar` incorrectly reading from an out-of-scope variable when called with the `(projectRef, slug, name, params)` overload. The `name` parameter was undefined in the implementation body; it now correctly reads from `nameOrRequestOptions`. |
There was a problem hiding this comment.
🟡 Release note text describes internals instead of user-facing behavior
The release note text added for this change describes internal implementation details (.changeset/fix-update-env-var-name-scope.md:5) rather than the user-visible behavior, which the repository guidelines require.
Impact: Users reading the published release notes see internal variable names instead of a plain description of what was fixed.
Repository rule from AGENTS.md on changeset wording
AGENTS.md states: "Write the description for users, not maintainers. Both changesets and .server-changes/ notes ship verbatim in user-visible release notes. Lead with what changed for the user - one plain sentence describing behavior, not implementation, and never naming internal tools or infra." The current text mentions "out-of-scope variable", "the implementation body" and the internal parameter name nameOrRequestOptions.
| Fix `updateEnvVar` incorrectly reading from an out-of-scope variable when called with the `(projectRef, slug, name, params)` overload. The `name` parameter was undefined in the implementation body; it now correctly reads from `nameOrRequestOptions`. | |
| Fixed updating an environment variable with an explicit project ref, slug and name sending an empty name, so the update now applies to the variable you specified. |
Was this helpful? React with 👍 or 👎 to provide feedback.
| $projectRef = projectRefOrName; | ||
| $slug = slugOrParams; | ||
| $name = name!; | ||
| $name = nameOrRequestOptions as string; |
There was a problem hiding this comment.
🔍 Second overload (name, params) still unusable outside a task context
Overload 2 declares update(name, params, requestOptions?). Outside a task context that call lands in the else branch at packages/trigger-sdk/src/v3/envvars.ts:327-329, where slugOrParams is the params object, so it throws "slug is required" — arguably correct (no project/env known), but the thrown message is misleading for that call shape. Worth confirming the intended error message/behaviour while touching this function.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
WalkthroughThe ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
1 participant
Closes #
✅ Checklist
Testing
The
updatefunction inenvvars.tshas three overloads:(projectRef, slug, name, params, requestOptions?)(projectRef, slug, params, requestOptions?)(projectRef, slugOrParams, nameOrRequestOptions?, params?, requestOptions?)In the implementation body, when overload 1 is matched (four args where the third is a string), the variable
nameis never declared as a parameter — the implementation signature usesnameOrRequestOptionsas its third positional. The code then writes:This resolves to
undefined, silently sending an undefined env var name to the API.Fix: read
nameOrRequestOptions(which correctly holds the name in this branch):To verify: call
updateEnvVar(projectRef, slug, "MY_VAR", { value: "x" })— before this fix,$nameisundefinedand the API call targets the wrong endpoint; after, it correctly targets/${projectRef}/${slug}/MY_VAR.Changelog
Fixed
updateEnvVarsilently usingundefinedas the env var name when called with the(projectRef, slug, name, params)overload. The implementation was reading from the wrong variable in scope.Screenshots
N/A