[ENG-14822][steps] Coerce env in steps to string#3583
[ENG-14822][steps] Coerce env in steps to string#3583szdziedzic wants to merge 1 commit intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the build-steps config validation to accept numeric env values in step definitions and coerce them to strings during Joi validation, aligning config parsing with how environment variables are ultimately represented.
Changes:
- Allow
envvalues in step configs to benumber | string, and coerce validated values to strings. - Update existing validation error-message assertions to match the new Joi alternatives type error.
- Add tests to verify numeric
envvalues are coerced to strings for bothrunsteps and function-call steps.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/steps/src/BuildConfig.ts | Expands Joi schema for env values to accept numbers and coerces validated values to strings. |
| packages/steps/src/tests/BuildConfig-test.ts | Updates error expectations and adds tests covering number-to-string coercion for env. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/steps/src/BuildConfig.ts
Outdated
| env: Joi.object().pattern( | ||
| Joi.string(), | ||
| Joi.alternatives() | ||
| .try(Joi.number(), Joi.string().allow('')) |
There was a problem hiding this comment.
With Joi's default convert: true, Joi.number() will coerce numeric-looking strings (e.g. '001', '1e3') into numbers before the .custom(String) runs, which can silently change env values (leading zeros, scientific notation, etc.). To avoid mutating string inputs, make the number branch strict (e.g. Joi.number().strict()) or prefer the string alternative and only accept actual numbers without coercing strings to numbers.
| .try(Joi.number(), Joi.string().allow('')) | |
| .try(Joi.number().strict(), Joi.string().allow('')) |
| command: 'echo 123', | ||
| env: { | ||
| HOMEBREW_NO_AUTO_UPDATE: 1, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Consider adding a regression test that a numeric-like string env value (e.g. PORT: '001') remains unchanged after validation. This will catch unintended Joi conversion where strings are coerced into numbers and then stringified, potentially losing formatting.
77ec396 to
543b32c
Compare
|
/changelog-entry bug-fix [steps] Coerce numeric env values to strings in workflow step configuration |
55c0633 to
1d0ea7c
Compare
|
Subscribed to pull request
Generated by CodeMention |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| test('env number coerced to string', () => { | ||
| const buildConfig = { | ||
| build: { | ||
| steps: [ | ||
| { | ||
| run: { | ||
| command: 'echo 123', | ||
| env: { | ||
| HOMEBREW_NO_AUTO_UPDATE: 1, | ||
| }, | ||
| }, | ||
| }, | ||
| ], | ||
| }, | ||
| }; | ||
|
|
||
| const config = validateConfig(BuildConfigSchema, buildConfig); | ||
| assert(isBuildStepCommandRun(config.build.steps[0])); | ||
| expect(config.build.steps[0].run.env).toEqual({ HOMEBREW_NO_AUTO_UPDATE: '1' }); | ||
| }); | ||
| test('numeric-like string env value is not coerced', () => { | ||
| const buildConfig = { | ||
| build: { | ||
| steps: [ | ||
| { | ||
| run: { | ||
| command: 'echo 123', | ||
| env: { | ||
| PORT: '001', | ||
| SCALE: '1e3', | ||
| }, | ||
| }, | ||
| }, | ||
| ], | ||
| }, | ||
| }; | ||
|
|
||
| const config = validateConfig(BuildConfigSchema, buildConfig); | ||
| assert(isBuildStepCommandRun(config.build.steps[0])); | ||
| expect(config.build.steps[0].run.env).toEqual({ PORT: '001', SCALE: '1e3' }); |
There was a problem hiding this comment.
PR description says there are two new test cases verifying numeric env coercion for both run steps and function calls, but this test file only adds coverage for run.env (and a separate case for numeric-like strings). Either add a test that asserts numeric env values are coerced for function-call steps (e.g. say_hi.env) or update the PR description/test plan to match what’s actually covered.
1d0ea7c to
d0f25b7
Compare
|
✅ Thank you for adding the changelog entry! |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3583 +/- ##
==========================================
+ Coverage 54.27% 54.28% +0.01%
==========================================
Files 820 820
Lines 35055 35056 +1
Branches 7260 7260
==========================================
+ Hits 19024 19025 +1
Misses 15944 15944
Partials 87 87 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|

Why
Passing numeric values in workflow step
env(e.g.env: { HOMEBREW_NO_AUTO_UPDATE: 1 }) results in a validation error because env values are required to be strings. YAML naturally parses unquoted numbers as numeric types, so users hit this unexpectedly.How
Updated the Joi env validation schema in
BuildFunctionCallSchemato accept both numbers and strings viaJoi.alternatives().try(Joi.number(), Joi.string().allow('')), with a.custom()callback that coerces numbers to strings viaString(value). This matches the coercion pattern used elsewhere in the codebase (e.g.stringLikein eas-cli).Booleans and objects are still rejected — booleans because YAML accepts both
Trueandtrue(coercing would lose casing), and objects because they aren't valid env values.Test Plan
"must be one of [number, string]"validation message.runcommands and function calls.