Skip to content

Set PACKAGE_MANAGER on the proxy container - #1773

Closed
AbhishekBhaskar wants to merge 1 commit into
mainfrom
abhishekbhaskar/set-package-manager-env-proxy
Closed

AbhishekBhaskar wants to merge 1 commit into
mainfrom
abhishekbhaskar/set-package-manager-env-proxy

Conversation

@AbhishekBhaskar

Copy link
Copy Markdown
Contributor

What & why

The Dependabot proxy runs in its own container and receives a curated set of env vars plus a credentials/experiments-only config.json — it never gets the full job.json. As a result os.Getenv("PACKAGE_MANAGER") was always empty in the proxy.

This is a prerequisite for the egress allowlist work: the allowlist keys its per-ecosystem defaults off PACKAGE_MANAGER, and the observability metric tags on it. Without this value, the allowlist degrades to GitHub-infra-only (every registry/CDN request would 403 under enforce), and package_manager shows empty in Datadog.

Note: dependabot-core was not affected — it already receives the full job (including package-manager) via job.json. This gap was specific to the proxy container.

Changes

  • src/updater.ts — forward details['package-manager'] into proxyBuilder.run(...).
  • src/proxy.tsrun() / createContainer() accept a packageManager param; add PACKAGE_MANAGER=${packageManager} to the proxy container Env.
  • dist/main.js — rebuilt (npm run package).
  • Tests updated + new assertion that PACKAGE_MANAGER is set on the container.

Testing

  • npm run typecheck, lint-check, format-check, package — all pass.
  • Unit tests (proxy/updater/main) — 102 passing, incl. new PACKAGE_MANAGER env test.
  • Integration tests not run (require Docker).

Related

@AbhishekBhaskar AbhishekBhaskar self-assigned this Sep 8, 2026
Copilot AI balanced review requested due to automatic review settings September 8, 2026 18:16
@AbhishekBhaskar
AbhishekBhaskar requested a review from a team as a code owner September 8, 2026 18:16

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟢 Approval recommended

The implementation consistently forwards the required value, updates all callers, and includes focused coverage.

Review tier: Balanced
Findings: None

What changed in this PR

Forwards the job’s package manager to the proxy container, enabling ecosystem-specific egress handling and observability.

Changes:

  • Passes package-manager through Updater and ProxyBuilder.
  • Sets PACKAGE_MANAGER in the proxy environment.
  • Updates unit/integration tests and bundled output.
File Description
src/​updater.ts Forwards the package manager.
src/​proxy.ts Sets the proxy environment variable.
dist/​main.js Updates bundled action code.
__tests__/​updater.test.ts Verifies argument forwarding.
__tests__/​updater-builder-integration.test.ts Updates proxy invocations.
__tests__/​proxy.test.ts Verifies the environment variable.
__tests__/​proxy-integration.test.ts Updates integration invocations.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@honeyankit honeyankit left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: dependabot-core was not affected — it already receives the full job (including package-manager) via job.json. This gap was specific to the proxy container.

@AbhishekBhaskar : My understanding is that, job.json will already contains package manager when it is passed to dependabot-action?

@AbhishekBhaskar

Copy link
Copy Markdown
Contributor Author

My understanding is that, job.json will already contains package manager when it is passed to dependabot-action?

@honeyankit yes correct, dependabot-api sends the JobDetails object to dependabot-action which then sends the job.json to dependabot-core. It does not send the job.json to proxy and the proxy only receives a few of the environment variables which does not include package-manager. However, we have removed this dependency on package-manager and are having a simple list of allowlisted domains in the proxy without any indexing by package-manager, so I'll be closing this PR out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants