Skip to content

test: drop bash login/rc loading in spawnSync invocations#3393

Merged
ericksoa merged 3 commits into
mainfrom
fix/test-bash-no-profile
May 13, 2026
Merged

test: drop bash login/rc loading in spawnSync invocations#3393
ericksoa merged 3 commits into
mainfrom
fix/test-bash-no-profile

Conversation

@laitingsheng
Copy link
Copy Markdown
Contributor

@laitingsheng laitingsheng commented May 12, 2026

Summary

Several test files use bash -lc / bash --norc -lc for one-off helpers (command -v lookups, sourcing local helper scripts, piping into mkfifo). The -l flag loads the user's login profile (/etc/profile, ~/.bash_profile, etc.). On developer machines whose shells source heavier per-login customisations, that adds noticeable startup cost per invocation, and with N invocations per test the cumulative wall time can exceed vitest's default 5s test timeout — even though each spawn does no useful work in login mode. Other invocations in the suite are paying the same tax silently because their per-test timeouts are higher than the wall time they currently use.

Switch host-side test spawns to bash --noprofile --norc -c: no login, no rc, equivalent functionality. Production code paths that intentionally run user-facing commands in login mode (onboard.ts, create-stream.ts, actions/update.ts) are left untouched, as are sandbox exec ... -- sh -lc invocations (those run inside the sandbox where login mode is required for correct PATH).

Changes

  • test/secret-redaction.test.ts: bash -lc 'command -v <name>' inside the 10-iteration symlink-building loop in the debug.sh wrapper test.
  • test/nemoclaw-start.test.ts: runGuardedOpenclaw helper. Already passes env: { ...process.env, PATH: ... }, so it never needed profile loading.
  • test/credentials.test.ts: two pipe-based prompt tests.
  • test/runtime-shell.test.ts: runShell helper used by many runtime-shell assertions.
  • test/smoke-macos-install.test.ts: mkfifo/IFS pipe pattern.
  • test/install-preflight.test.ts: two command -v python3 lookups.

Type of Change

  • Code change (feature, bug fix, or refactor)
  • Code change with doc updates
  • Doc only (prose changes, no code sample modifications)
  • Doc only (includes code sample changes)

Verification

  • npx prek run --all-files passes
  • npm test passes
  • Tests added or updated for new or changed behavior
  • No secrets, API keys, or credentials committed
  • Docs updated for user-facing behavior changes
  • make docs builds without warnings (doc changes only)
  • Doc pages follow the style guide (doc changes only)
  • New doc pages include SPDX header and frontmatter (new pages only)

Signed-off-by: Tinson Lai tinsonl@nvidia.com

Summary by CodeRabbit

  • Tests
    • Refined shell initialization behavior across test suites to improve test consistency and reliability.

Review Change Stack

Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
@laitingsheng laitingsheng added the enhancement: testing Use this label to identify requests to improve NemoClaw test coverage. label May 12, 2026
@copy-pr-bot
Copy link
Copy Markdown

copy-pr-bot Bot commented May 12, 2026

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 12, 2026

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: converting bash invocations from login/rc loading (-lc) to non-login/non-rc mode (--noprofile --norc -c) across multiple test files.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/test-bash-no-profile

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 12, 2026

E2E Advisor Recommendation

Required E2E: None
Optional E2E: macos-e2e, test-e2e-ollama-proxy

Workflow run

Full advisor summary

Pi Semantic E2E Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required E2E

  • None. Changes are confined to vitest unit-test files and only swap bash -lc for bash --noprofile --norc -c inside spawnSync harness calls. No production source (bin/, scripts/, src/, nemoclaw/), workflows, installer payloads, credential store, gateway/runtime scripts, or E2E scenarios are modified. The one edit inside a describe.skip/it.skip block is inert. There is no user-facing, security-boundary, networking, credential, installer, sandbox-lifecycle, inference-routing, or deployment behavior change that could break at runtime, so no E2E suite is merge-blocking.

Optional E2E

  • macos-e2e (low): Several of the altered tests (install-preflight, nemoclaw-start configure guard, runtime-shell Colima/Docker-Desktop detection, smoke-macos-install) exercise bash-sourced installer/runtime logic that behaves differently on macOS shells. Although no production code changed, running macos-e2e once confirms the vitest suite still passes on macOS runners where login-shell profiles most commonly leak state — this is the explicit motivation for the --noprofile --norc switch. Not merge-blocking.
  • test-e2e-ollama-proxy (low): Default PR E2E gate already runs via .github/workflows/pr.yaml. Keeping it as an optional sanity check costs nothing and exercises the shared bash harness patterns in CI. Not required since no production paths change.

New E2E recommendations

  • ci-shell-hermeticity (low): The PR motivates itself on avoiding contamination from login shell profiles, but there is no repo-level lint or CI guard that prevents future tests (or production scripts under scripts/) from reintroducing bash -lc / bash -l spawns where hermeticity matters. A lightweight grep-based check in a static-analysis workflow would lock the invariant in place.
    • Suggested test: Add a static CI check (e.g. in .github/workflows/pr.yaml checks job) that greps test/**/*.ts and scripts/**/*.sh for bash\s+-lc\b / bash\s+-l\b spawnSync usage and fails with a pointer to bash --noprofile --norc -c, with an explicit allowlist for cases that intentionally want login semantics.

Dispatch hint

  • Workflow: .github/workflows/pr.yaml
  • jobs input: ``

Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
@laitingsheng laitingsheng marked this pull request as ready for review May 12, 2026 07:59
@laitingsheng laitingsheng added the v0.0.40 Release target label May 13, 2026
Copy link
Copy Markdown
Contributor

@ericksoa ericksoa left a comment

Choose a reason for hiding this comment

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

Reviewed the test-only bash invocation cleanup. I verified the focused changed-test suite and CLI build locally; product regression risk is very low because this only changes host-side test harness shell startup.

@ericksoa ericksoa merged commit 6f4e276 into main May 13, 2026
28 of 30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement: testing Use this label to identify requests to improve NemoClaw test coverage. v0.0.40 Release target

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants