fix(cli): match Go bundler env and deploy path anchoring (CLI-1985) - #6005
Conversation
Two functions deploy parity fixes against the pinned Go CLI: - Stop forwarding NPM_AUTH_TOKEN into the eszip Docker bundler container; Go forwards only NPM_CONFIG_REGISTRY (bundle.go:68-70). This reverts the TS-only forwarding from #5645 (the Go-side #4933 was closed unmerged). BREAKING for private-registry users whose .npmrc expands NPM_AUTH_TOKEN during --use-docker deploys: inline the token in .npmrc or deploy via --use-api instead. - Anchor API-deploy uploaded file names and the server-recorded entrypoint_path/import_map_path/static_patterns at the workdir, matching Go's toRelPath (relative to os.Getwd(), forward slashes). The git-root import-walk boundary from #5755 is kept, so monorepo imports outside the workdir still deploy, now with Go-style ../-relative names.
|
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
…5-functions-deploy-rulings-npm_auth_token-forwarding-and-git
Supabase CLI previewnpx --yes https://pkg.pr.new/supabase/cli/supabase@e1b18f3d479f1fca5f2a6a21bdbb9e61f7fe9850Preview package for commit |
kanadgupta
left a comment
kanadgupta
left a comment
There was a problem hiding this comment.
Claude discovered a potential issue where the server might reject the file name and suggests trying this yourself — I think this would be better suited for the e2e tests I'm hoping to work on soon. Will defer to you on the best course of action here — LGTM otherwise!
…ir (CLI-1985) Go's writeForm/addFile opens every uploaded path through an fs.FS, which rejects any path containing a ".." element via fs.ValidPath before the read (and thus the upload) happens. A workdir≠git-root layout could make the TS deploy path anchoring produce a multipart File name like "../packages/shared/src/index.ts" that escapes the workdir and reaches the server for the first time from any CLI. Hard-fail with the same Go-parity error before any upload is attempted, and update the deploy integration tests that had been asserting the escaping upload succeeded.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e1b18f3d47
ℹ️ 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".
2 participants
Two
functions deploydivergences from the pinned Go CLI (apps/cli-go), resolved per the CLI-1985 ruling (Colum, 2026-07-30: take each point's documented recommended option).Fixes CLI-1985
⚖ Parity ruling applied
Point 1 —
NPM_AUTH_TOKENis no longer forwarded into the Docker bundler (strict parity; breaking for private-registry users)Decision: remove the forwarding. The eszip bundler container now receives only
NPM_CONFIG_REGISTRYfrom the host, exactly matching Go (apps/cli-go/internal/functions/deploy/bundle.go:68-70).Evidence:
NPM_AUTH_TOKENat any point in its history — onlyNPM_CONFIG_REGISTRY(added in8e17f033).functions deployfails to read.npmrcenvironment variables for private npm registry auth (workspacedeno.json) #4927) was closed unmerged on 2026-06-22 ("The command is now ported in TypeScript so I'm closing this PR").User-visible change (flagging prominently): users whose
.npmrcexpands${NPM_AUTH_TOKEN}for private npm registries will find--use-docker/--legacy-bundledeploys failing registry auth again (the pre-#5645 and Go CLI behavior; re-opens the CI/CD-host case of #4927). Workarounds: inline the token in.npmrc, or deploy via the default--use-apipath. Per the strict-parity contract (stderr bytes included), no TS-only warning was added when the variable is set — a DX reviewer requested one and it was rejected on parity grounds; the breaking impact is documented here and in the commit message instead.Shared-code caveat (per the ruling):
dockerNpmEnvlives inapps/cli/src/shared/functions/deploy.tsand serves both shells —next/(functions deploy) andlegacy/. The removal therefore applies to the next/ shell too. The strict-parity contract only binds the legacy shell, but keeping one code path is the simplest correct design per repo policy, so next/ loses the forwarding as well — stated here explicitly.functions serveis unaffected (it has its own env handling, matching Go's serve which loadssupabase/functions/.env).Point 2 — API-deploy upload paths re-anchored at the workdir (align to the pinned oracle; behaviour change)
Directive: confirm the intended reference point first, then align or record.
Evidence found:
pkg/function/deploy.go(pre- and post-monorepo move) showstoRelPathanchored atos.Getwd()since29021998("convert all paths to relative for deploy", fix: convert all paths to relative for deploy #3403), unchanged since. The Go CLI chdirs to the workdir (internal/utils/misc.go:238), soos.Getwd()≡ the workdir.failed to read file: open ../common/index.ts) — not a port of newer upstream Go behavior. There is no newer upstream Go reference to record against.Decision (per the ruling's matrix — upstream never did this → align): uploaded multipart file names and the server-recorded
entrypoint_path/import_map_path/static_patternsare now anchored at the workdir with Go's exacttoRelPathsemantics (relative toos.Getwd(), forward slashes,../-relative when the file lies outside the workdir).Scope note: #5755's import-walk containment boundary (which files may be uploaded at all) is intentionally kept at the nearest git root. The boundary is a TS-only safeguard with no Go equivalent — Go's walker uploads any reachable import unbounded (and then hard-fails opening
..paths throughafero.NewIOFS, which is exactly bug #3467). Reverting the boundary would re-break #3467 and is outside CLI-1985's anchoring scope.User-visible change: in monorepos where the git root is an ancestor of the workdir, redeploys now record
supabase/functions/<slug>/index.ts(matching what the Go CLI records and the dashboard shows for Go deploys) instead ofapps/myapp/supabase/functions/<slug>/index.ts. Imports outside the workdir but inside the git root still deploy, uploading with Go-style../-relative names — the same name shape Go'stoRelPathemits, so nothing new is required of the server. Non-monorepo projects (git root == workdir, the common case) are byte-for-byte unchanged.What changed
apps/cli/src/shared/functions/deploy.ts—dockerNpmEnvNamestrimmed toNPM_CONFIG_REGISTRY;deployViaApinow threads the workdir as the path anchor throughuploadFunctionSource/writeSourceDeployForm/createSourceMetadatawhile the git-rootsourceRootremains the containment boundary; ENOENT warn display paths follow the workdir anchor (matching Go's workdir-relative walker paths). Docker bind construction is untouched.apps/cli/src/legacy/commands/functions/deploy/deploy.integration.test.ts— new regression test: workdir≠git-root monorepo deploy asserts workdir-anchored metadata,../-relative upload names, and the Go-parityUploading assetstderr line.apps/cli/src/next/commands/functions/deploy/deploy.integration.test.ts— the two git-root upload tests updated to the workdir anchoring; the npm env test now assertsNPM_CONFIG_REGISTRYis forwarded andNPM_AUTH_TOKENis not.apps/cli/src/legacy/commands/functions/deploy/SIDE_EFFECTS.md— env table states onlyNPM_CONFIG_REGISTRYis forwarded; new note documents the workdir anchoring and the TS-only git-root boundary.All four changed/added tests fail against the previous implementation and pass with this change.