feat: support isSecret in syncEnvVars#4203
Conversation
🦋 Changeset detectedLatest commit: 0633aae The changes in this PR will be included in the next version bump. This PR includes changesets to release 28 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 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📜 Recent review details⏰ Context from checks skipped due to timeout. (41)
🧰 Additional context used📓 Path-based instructions (4)**/*.{ts,tsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.ts📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc)
Files:
packages/cli-v3/src/commands/**/*📄 CodeRabbit inference engine (packages/cli-v3/CLAUDE.md)
Files:
🧠 Learnings (9)📚 Learning: 2026-03-22T13:26:12.060ZApplied to files:
📚 Learning: 2026-03-22T19:24:14.403ZApplied to files:
📚 Learning: 2026-05-18T08:21:27.694ZApplied to files:
📚 Learning: 2026-05-18T08:21:27.694ZApplied to files:
📚 Learning: 2026-06-13T19:53:13.759ZApplied to files:
📚 Learning: 2026-06-17T17:13:49.929ZApplied to files:
📚 Learning: 2026-06-23T13:04:21.413ZApplied to files:
📚 Learning: 2026-06-04T18:16:35.386ZApplied to files:
📚 Learning: 2026-06-09T17:58:04.699ZApplied to files:
🔇 Additional comments (2)
WalkthroughThis PR adds support for marking environment variables synced via 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
@trigger.dev/build
trigger.dev
@trigger.dev/core
@trigger.dev/python
@trigger.dev/react-hooks
@trigger.dev/redis-worker
@trigger.dev/rsc
@trigger.dev/schema-to-json
@trigger.dev/sdk
commit: |
There was a problem hiding this comment.
Devin Review found 2 potential issues.
🐛 1 issue in files not directly in the diff
🐛 Secret environment variables are silently dropped during worker/unmanaged builds (packages/cli-v3/src/commands/workers/build.ts:262-279)
Secret env vars are never forwarded to the server during worker builds (syncEnvVarsWithServer at packages/cli-v3/src/commands/workers/build.ts:449-463) because this code path was not updated, so any variable marked isSecret: true is silently lost.
Impact: Users deploying via the unmanaged/worker build path will have their secret environment variables silently ignored, leading to missing secrets at runtime.
Incomplete transformation: deploy.ts was updated but workers/build.ts was not
The PR updated packages/cli-v3/src/commands/deploy.ts:766-814 to split secret and non-secret env vars into separate API calls and pass secretEnvVars/secretParentEnvVars to syncEnvVarsWithServer. However, packages/cli-v3/src/commands/workers/build.ts has its own parallel copy of both the sync detection logic and the syncEnvVarsWithServer function, and neither was updated.
Three specific issues:
-
The sync detection condition at
packages/cli-v3/src/commands/workers/build.ts:262-265only checksbuildManifest.deploy.sync.env— if all vars are secret (no non-secret vars exist), the entire sync block is skipped. -
The call at
packages/cli-v3/src/commands/workers/build.ts:273-278only passesenvandparentEnv, notsecretEnvorsecretParentEnv. -
The function at
packages/cli-v3/src/commands/workers/build.ts:449-463doesn't accept or handle secret env var parameters at all, unlike its updated counterpart indeploy.ts.
| if (layer.deploy.override || $manifest.deploy.env[key] === undefined) { | ||
| const existingValue = $manifest.deploy.env[key]; | ||
|
|
||
| if (existingValue !== value) { | ||
| $manifest.deploy.sync.secretEnv[key] = value; | ||
| } | ||
| } |
There was a problem hiding this comment.
🔍 Secret env dedup logic checks non-secret env map, which may skip valid secret overrides
In applyLayerToManifest at packages/cli-v3/src/build/extensions.ts:188-226, the override check for secretEnv and secretParentEnv uses $manifest.deploy.env[key] — the non-secret env map. This means if a key already exists as a non-secret env var with the same value, the secret version won't be added to sync.secretEnv. This is consistent with how the existing non-secret env/parentEnv dedup works, but it creates an implicit coupling: a user cannot "upgrade" an existing non-secret var to secret via syncEnvVars if the value hasn't changed, because the dedup check will suppress it. This edge case seems unlikely in practice but is worth being aware of.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Intentional — mirrors the existing non-secret dedup so behavior is uniform. The "upgrade unchanged value to secret" edge case is niche and can be done via the dashboard.
What
Adds per-variable secret support to the
syncEnvVarsbuild extension. Return{ name, value, isSecret: true }and the variable is stored as a secret (redacted in the dashboard, value non-revealable), just like a manually created secret env var. Secret and non-secret variables can be mixed in one callback.How
Env vars flow through the build pipeline as a flat name→value map, and the import API's
isSecretis per-call. So secret vars are carried through the layer + manifest in parallelsecretEnv/secretParentEnvmaps, and at deploy time they go up in a secondimportEnvVarscall withisSecret: true(the plain vars in the first call). The record form ({ KEY: "value" }) is unchanged and stays non-secret.Commits
feat(core): carry secret env vars through the build layer + manifest schemafeat(build): partitionisSecretvars insyncEnvVarsfeat(cli): merge secret layers and import them withisSecret: trueat deploytest(build): cover the partitioning + documentisSecretTesting
isSecret: true) and the plain var visible.Closes TRI-11099