[APPS-2792] Add: v3 to v4 migration guide entry - #511
Conversation
This comment has been minimized.
This comment has been minimized.
5395c22 to
c01b3b9
Compare
4687e4c to
b39e663
Compare
b39e663 to
281ac50
Compare
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
🟡 Changes recommended
Documentation corrections remain in MIGRATIONS.md.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
MIGRATIONS.md:52
- The phrase “even though the equivalent read against the deployed function succeeds in production” is too broad: an arbitrary shell variable such as the
AWS_REGIONexample is not necessarily configured in the deployed app at all. Qualify this with “when that variable is provided by the deployed app or a connection” so the guide does not imply production supplies every local shell variable.
Reading any other variable — including one your shell has set, or one a secret-backed connection would resolve to in production — returns `undefined` locally, even though the equivalent read against the deployed function succeeds in production. The one exception is a Custom Credentials-backed variable declared in `datadog-app.local.json` (see below).
MIGRATIONS.md:36
- This migration entry omits the other half of the breaking behavior: local execution installs runtime guards for network, subprocess, worker-thread, and DNS-resolver APIs, so a dependency can now fail when it makes one even if the app file has no forbidden syntax. Please document that alongside the build-time checks; literal dynamic Node imports are also rejected, so “static imports” is too narrow.
Static imports of Node built-ins (`fs`, `child_process`, `net`, etc.) and raw network globals (`fetch`, `XMLHttpRequest`, `WebSocket`, `EventSource`) in a backend file are rejected at build time. Backend functions have never had access to these in production, so this only surfaces earlier — at `npm run dev` time instead of only once the app is published — a case that previously appeared to work locally but would fail in production.
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 281ac50508
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
10d55b1 to
e3c9bda
Compare
Documents the upcoming breaking change to how `npm run dev` runs backend functions: in-process execution instead of a cloud round trip, the new `npm run dev:verify` cloud-parity check, and the process.env allowlist during local execution.
e3c9bda to
3e35c51
Compare
Motivation
npm run dev:verify, the runtime network/subprocess guard, build-time rejection of Node built-ins/raw network globals, Custom Credentials local resolution, and real-identity hydration forgetInitiatingUser()/getExecutionUser()), withprocess.envscoping as the last piece still in review.majorversion bump —MIGRATIONS.mdneeds a## v3 to v4entry describing it before that bump goes out.bump.yamldispatch) should wait until theprocess.envscoping work and its security sign-off land — see Out of Scope below.Changes
2 changes across MIGRATIONS.md
## v3 to v4entry covering in-process local execution,npm run dev:verify, and theprocess.envallowlist, plus its Table of Contents entryMIGRATIONS.mddatadog-app.local.json) and real-identity hydration forgetInitiatingUser()/getExecutionUser(), plus their Table of Contents entriesMIGRATIONS.mdQA Instructions
Docs-only change — no functional QA applies.
The described behavior was cross-checked against the actual merged code rather than assumed:
/__dd/executeActionvs./__dd/executeActionViaClouddev-server routing:packages/plugins/apps/src/vite/dev-server.ts,packages/plugins/apps/src/vite/local-execution.tsnpm run dev:verify/DEV_VERIFY_MODErouting:packages/plugins/apps/src/constants.ts,packages/plugins/apps/src/vite/dev-server.tsgh pr diff 476 --repo DataDog/build-pluginsprocess.envallowlist (SAFE_ENV_KEYS = ['PATH', 'HOME', 'NODE_ENV', 'TMPDIR']): read directly off the still-opentiffany.trinh/apps-2792-env-guard-hardeningbranch'spackages/plugins/apps/src/vite/env-guard.ts, sinceenv-guard.tsisn't onmasteryetresolveCustomCredentials, the sanitized JSON-parse error that never echoes file content) and real-identity hydration (RuntimeUser,assertValidRuntimeContext): read directly offpackages/plugins/apps/src/vite/custom-credentials-resolver.tsandpackages/plugins/apps/src/vite/local-execution.tsBlast Radius
MIGRATIONS.md). Zero runtime risk.Out of Scope / Follow-ups
1 item deferred
majorversion bump (bump.yamldispatch) this entry documents