Skip to content

feat(sandbox): add Rscript support to environment configuration#490

Closed
zieen wants to merge 1 commit intoNVIDIA:mainfrom
yichozy:feat-add-rscript-support
Closed

feat(sandbox): add Rscript support to environment configuration#490
zieen wants to merge 1 commit intoNVIDIA:mainfrom
yichozy:feat-add-rscript-support

Conversation

@zieen
Copy link

@zieen zieen commented Mar 20, 2026

Add documentation and test coverage for Rscript language support in sandbox child process environment variables. Rscript benefits from existing lowercase proxy variables and libcurl/OpenSSL TLS certificate bundles.

Changes:

  • Document Rscript compatibility with http_proxy/https_proxy
  • Document SSL_CERT_FILE coverage for R and other OpenSSL tools
  • Document CURL_CA_BUNDLE for R httr package
  • Add test assertions verifying lowercase proxy vars
  • Add test assertion verifying CURL_CA_BUNDLE

Summary

Related Issue

Changes

Testing

  • mise run pre-commit passes
  • Unit tests added/updated
  • E2E tests added/updated (if applicable)

Checklist

  • Follows Conventional Commits
  • Commits are signed off (DCO)
  • Architecture docs updated (if applicable)

Add documentation and test coverage for Rscript language support
in sandbox child process environment variables. Rscript benefits
from existing lowercase proxy variables and libcurl/OpenSSL TLS
certificate bundles.

Changes:
- Document Rscript compatibility with http_proxy/https_proxy
- Document SSL_CERT_FILE coverage for R and other OpenSSL tools
- Document CURL_CA_BUNDLE for R httr package
- Add test assertions verifying lowercase proxy vars
- Add test assertion verifying CURL_CA_BUNDLE
@zieen zieen requested a review from a team as a code owner March 20, 2026 00:40
Copilot AI review requested due to automatic review settings March 20, 2026 00:40
@github-actions
Copy link

Thank you for your interest in contributing to OpenShell, @zieen.

This project uses a vouch system for first-time contributors. Before submitting a pull request, you need to be vouched by a maintainer.

To get vouched:

  1. Open a Vouch Request discussion.
  2. Describe what you want to change and why.
  3. Write in your own words — do not have an AI generate the request.
  4. A maintainer will comment /vouch if approved.
  5. Once vouched, open a new PR (preferred) or reopen this one after a few minutes.

See CONTRIBUTING.md for details.

@github-actions github-actions bot closed this Mar 20, 2026
@github-actions
Copy link

Thank you for your submission! We ask that you sign our Developer Certificate of Origin before we can accept your contribution. You can sign the DCO by adding a comment below using this text:


I have read the DCO document and I hereby sign the DCO.


You can retrigger this bot by commenting recheck in this Pull Request. Posted by the DCO Assistant Lite bot.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR improves sandbox child-process environment configuration clarity and coverage for Rscript by documenting and testing the relevant proxy and TLS certificate bundle environment variables.

Changes:

  • Document Rscript compatibility with lowercase http_proxy/https_proxy and libcurl/OpenSSL bundle env vars.
  • Extend unit tests to assert lowercase proxy variables are present.
  • Extend TLS env var test to assert CURL_CA_BUNDLE is set.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 18 to 21
// Node.js only honors HTTP(S)_PROXY for built-in fetch/http clients when
// proxy support is explicitly enabled at process startup.
// Rscript automatically respects lowercase http_proxy/https_proxy.
("NODE_USE_ENV_PROXY", "1".to_owned()),
Copy link

Copilot AI Mar 20, 2026

Choose a reason for hiding this comment

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

The new Rscript note is placed immediately above NODE_USE_ENV_PROXY, which could imply it relates to that setting. Since it documents the lowercase http_proxy/https_proxy entries, consider moving this comment next to those env var entries (or splitting into separate comments for Node vs. lowercase proxies) to keep the rationale aligned with the relevant keys.

Copilot uses AI. Check for mistakes.
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.

2 participants