Skip to content

fix(cli): isolate subprocess tests from host OPENSHELL_ env vars - #2523

Merged
elezar merged 1 commit into
NVIDIA:mainfrom
rhuss:fix/cli-subprocess-test-env-isolation
Jul 29, 2026
Merged

fix(cli): isolate subprocess tests from host OPENSHELL_ env vars#2523
elezar merged 1 commit into
NVIDIA:mainfrom
rhuss:fix/cli-subprocess-test-env-isolation

Conversation

@rhuss

@rhussrhuss commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Summary

The subprocess integration tests inherit the full parent environment. If the developer has OPENSHELL_GATEWAY_INSECURE=true set in their shell, the spawned CLI process connects with .with_no_client_auth(), skipping the mTLS client certificate. The test server requires mTLS and responds with CertificateRequired.

Related Issue

Follow-up to #2504. Root cause identified by @sjenning.

Changes

  • Strip OPENSHELL_GATEWAY_INSECURE, OPENSHELL_GATEWAY, OPENSHELL_GATEWAY_ENDPOINT, and OPENSHELL_WORKSPACE from the subprocess environment via env_remove()
  • The test already sets --gateway and --gateway-endpoint explicitly via CLI args, so these env vars should not influence subprocess behavior

Testing

  • mise run pre-commit passes
  • Unit tests added/updated
  • Verified: test fails with OPENSHELL_GATEWAY_INSECURE=true before fix, passes after
  • Full test suite (22/22) passes with and without the env var set
  • E2E tests added/updated (if applicable)

Checklist

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

@copy-pr-bot

copy-pr-botBot commented Jul 28, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

elezar
elezar reviewed Jul 28, 2026
View reviewed changes
Comment threadcrates/openshell-cli/tests/sandbox_create_lifecycle_integration.rs Outdated
@rhuss
fix(cli): isolate subprocess tests from host OPENSHELL_ env vars
d71c2ac
The subprocess-based integration tests inherit the full parent
environment. If the developer has OPENSHELL_GATEWAY_INSECURE=true
set in their shell, it leaks into the spawned CLI process and causes
it to connect with .with_no_client_auth(), skipping the mTLS client
certificate. The test server requires mTLS, so it responds with
CertificateRequired and the test fails.
Strip OPENSHELL_GATEWAY_INSECURE, OPENSHELL_GATEWAY,
OPENSHELL_GATEWAY_ENDPOINT, and OPENSHELL_WORKSPACE from the
subprocess environment. The test already sets --gateway and
--gateway-endpoint explicitly via CLI args, so these env vars
should not influence the subprocess behavior.
Reported-by: Seth Jennings
Signed-off-by: Roland Huß <rhuss@redhat.com>
@rhuss
rhussforce-pushed the fix/cli-subprocess-test-env-isolation branch from d93fc33 to d71c2acCompareJuly 28, 2026 16:34
@elezarelezar added the test:e2e Requires end-to-end coverage label Jul 29, 2026
@github-actions

github-actionsBot commented Jul 29, 2026

Copy link
Copy Markdown

Label test:e2e applied, but pull-request/2523 does not exist yet. A maintainer needs to comment /ok to test d71c2ace89dc176bb0fcfca4bc62cdc08b799280 to mirror this PR. Once the mirror exists, re-apply the label or re-run Branch E2E Checks from the Actions tab.

@elezar

elezar commented Jul 29, 2026

Copy link
Copy Markdown
Member

/ok-to-test d71c2ac

elezar
elezar approved these changes Jul 29, 2026
View reviewed changes

@elezarelezar left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @rhuss. I think being explicit will be better in the long run.

@elezarelezar added the test:e2e-gpu Requires GPU end-to-end coverage label Jul 29, 2026
@github-actions

github-actionsBot commented Jul 29, 2026

Copy link
Copy Markdown

Label test:e2e-gpu applied for d71c2ac. Open the existing run and click Re-run all jobs to execute with the label set. The run will execute GPU E2E after building the required supervisor image once. The matching required CI gate status on this PR will flip green automatically once the run finishes.

@elezar
elezar added this pull request to the merge queueJul 29, 2026
Merged via the queue into NVIDIA:main with commit 1221b58Jul 29, 2026
91 checks passed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@elezarelezarelezar approved these changes
@mrunalpmrunalpAwaiting requested review from mrunalpmrunalp is a code owner
@maxamillionmaxamillionAwaiting requested review from maxamillion
@derekwaynecarrderekwaynecarrAwaiting requested review from derekwaynecarrderekwaynecarr is a code owner

Assignees

No one assigned

Labels

test:e2eRequires end-to-end coveragetest:e2e-gpuRequires GPU end-to-end coverage

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@rhuss@elezar