Skip to content

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

Queued
rhuss wants to merge 1 commit into
NVIDIA:mainfrom
rhuss:fix/cli-subprocess-test-env-isolation
Queued

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

Conversation

@rhuss

@rhuss rhuss 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)

@rhuss
rhuss requested review from a team, derekwaynecarr, maxamillion and mrunalp as code owners July 28, 2026 16:00
@copy-pr-bot

copy-pr-bot Bot 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.

Comment thread crates/openshell-cli/tests/sandbox_create_lifecycle_integration.rs Outdated
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
rhuss force-pushed the fix/cli-subprocess-test-env-isolation branch from d93fc33 to d71c2ac Compare July 28, 2026 16:34
@elezar elezar added the test:e2e Requires end-to-end coverage label Jul 29, 2026
@github-actions

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 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.

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

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 queue Jul 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

test:e2e Requires end-to-end coverage test:e2e-gpu Requires GPU end-to-end coverage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants