test: clean up temporary files after runs - #6690
Conversation
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
📝 WalkthroughWalkthroughVitest now initializes an isolated temporary root for test runs. Test harnesses replace fixed temporary paths with dynamically generated directories, add cleanup assertions, and isolate installer, workflow checkout, sandbox, service, and temporary-home state. ChangesTest isolation and portability
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage remains at 96%, unchanged from the TypeScript / code-coverage/cliThe overall coverage in the Show a code coverage summary of the most impacted files.
Updated |
E2E Advisor RecommendationRequired E2E: None Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
test/e2e-advisor.test.ts (1)
78-83: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid locking this test to the placeholder implementation.
gitCallsandgithubEnvalready prove that the per-run directory reaches the workflow boundary, soexpect(workflowScript).toContain(E2E_ADVISOR_TARGET_DIR)unnecessarily couples the test to the workflow’s literal placeholder. Remove this assertion and retain the behavioral checks. As per path instructions, tests should prioritize behavioral confidence over implementation lock-in.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e-advisor.test.ts` around lines 78 - 83, Remove the expect(workflowScript).toContain(E2E_ADVISOR_TARGET_DIR) assertion from the test, while retaining the gitCalls and githubEnv behavioral checks that verify the per-run directory reaches the workflow boundary.Source: Path instructions
test/pr-review-advisor-workflow-boundary.test.ts (1)
51-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid locking this test to the placeholder implementation.
gitCallsandgithubEnvalready prove that the per-run directory reaches the workflow boundary, soexpect(workflowScript).toContain(PR_REVIEW_ADVISOR_TARGET_DIR)unnecessarily couples the test to the workflow’s literal placeholder. Remove this assertion and retain the behavioral checks. As per path instructions, tests should prioritize behavioral confidence over implementation lock-in.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/pr-review-advisor-workflow-boundary.test.ts` around lines 51 - 56, Remove the implementation-specific expect(workflowScript).toContain(PR_REVIEW_ADVISOR_TARGET_DIR) assertion from the boundary test, while retaining the gitCalls and githubEnv behavioral checks that verify the per-run directory reaches the workflow boundary.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/install-stage-from-stdin.test.ts`:
- Around line 26-31: The test helper runEntryGuardInFixture duplicates the
installer’s entry-guard logic and bypasses its EXIT-trap cleanup. Replace this
copied harness with a contract test that executes scripts/install.sh using only
a controlled curl/input setup, then assert the real installer removes the staged
path during EXIT cleanup; alternatively extract and invoke one shared
entry-guard implementation so the test cannot drift from production.
In `@test/vitest-temp-root.test.ts`:
- Around line 27-35: Remove the added if statements from restoreTempEnv and the
related finally cleanup paths in this test file while preserving identical
environment restoration and cleanup behavior. Refactor the branching through an
approved shared helper or equivalent expression-based logic, including all
locations noted by the review, so the codebase-growth guard no longer reports
these five if statements.
---
Nitpick comments:
In `@test/e2e-advisor.test.ts`:
- Around line 78-83: Remove the
expect(workflowScript).toContain(E2E_ADVISOR_TARGET_DIR) assertion from the
test, while retaining the gitCalls and githubEnv behavioral checks that verify
the per-run directory reaches the workflow boundary.
In `@test/pr-review-advisor-workflow-boundary.test.ts`:
- Around line 51-56: Remove the implementation-specific
expect(workflowScript).toContain(PR_REVIEW_ADVISOR_TARGET_DIR) assertion from
the boundary test, while retaining the gitCalls and githubEnv behavioral checks
that verify the per-run directory reaches the workflow boundary.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: fd829377-bf80-41bb-a3f8-6ac616b59c9c
📒 Files selected for processing (13)
nemoclaw/vitest.config.tssrc/lib/tunnel/services.test.tstest/e2e-advisor.test.tstest/helpers/vitest-temp-root.tstest/install-preflight.test.tstest/install-stage-from-stdin.test.tstest/nemo-deepagents-alias.test.tstest/nemohermes-alias.test.tstest/pr-review-advisor-workflow-boundary.test.tstest/runtime-shell.test.tstest/service-env.test.tstest/vitest-temp-root.test.tsvitest.config.ts
PR Review Advisor — No blocking findingsMerge posture: No blocking advisor findings This is an automated review. Required findings need action before merge. Warnings and optional suggestions do not require a response or follow-up. A human maintainer makes the final merge decision. |
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/install-preflight.test.ts`:
- Around line 401-403: Update the stagedFixturePath construction in the test
setup to create the fixture under the existing per-test temporary directory
represented by tmp, rather than hardcoding /tmp. Preserve the unique filename
and exclusive write behavior in the writeFileSync call so run-scoped cleanup
includes the staged fixture.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5a3a6633-de48-42ee-89c1-d220650a0963
📒 Files selected for processing (2)
test/install-preflight.test.tstest/vitest-temp-root.test.ts
<!-- markdownlint-disable MD041 --> ## Summary Vitest runs now use one private, run-scoped temp root that is removed automatically after the run, including the process-exit fallback path. Test fixtures that previously bypassed the configured temp directory now use owned, unique paths and explicit cleanup. Set `NEMOCLAW_TEST_KEEP_TEMP=1` to retain the run root for debugging. ## Changes - Add shared Vitest global setup for the root projects and standalone plugin suite. - Redirect `TMPDIR`, `TMP`, and `TEMP` into the run root, then restore the caller environment during teardown. - Clean installer, service, alias, advisor, and runtime-shell fixtures that previously escaped or outlived their tests. - Add regression coverage for recursive cleanup, caller-environment restoration, keep-temp behavior, and config wiring. ## Type of Change - [x] 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) ## Quality Gates - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [ ] Docs updated for user-facing behavior changes - [x] Docs not applicable — justification: test infrastructure and fixtures only; no user-facing CLI, configuration, API, policy, or sandbox behavior changed - [ ] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [ ] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Verification - [x] PR description includes the DCO sign-off declaration and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run check:diff` passed when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — 124 integration tests, 35 CLI tests, 1 focused installer test, and 9 standalone plugin tests passed; keep-temp mode passed 4/4; `npm run test:projects:check` passed for 1,483 files - [ ] Applicable broad gate passed — `npm test` exercised 1,406 files; two aggregate-only failures passed in exact CI-mode isolation (10/10 and 64/64), so PR CI is the authoritative broad gate - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `npm run docs` builds without warnings (doc changes only) - [ ] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) --- Signed-off-by: Carlos Villela <cvillela@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Improved isolation by using dynamically created, OS-appropriate temp directories/files instead of fixed `/tmp` paths. * Added a shared temp-root initializer for tests, with robust teardown, environment restoration, and optional temp retention. * Updated e2e/installer/checkout/service/runtime tests to use dynamic target and sandbox directories and to ensure staged files and temp artifacts are cleaned up. * **Tooling** * Configured the test runner to perform global temp initialization before executing tests. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Carlos Villela <cvillela@nvidia.com>
## Summary - raise the Hermes Python guard test-harness allowance from 5 seconds to 90 seconds - apply the shared allowance to the three runtime-config-guard helper paths only - correct the WSL root-only peer fixture to start from the production mutable topology, `sandbox:sandbox 03770` - assert sealing produces `root:sandbox 03770`, permits sandbox-group runtime-state creation, rejects unlink of root-owned sealed config, and restores the original ownership - let the existing config-reclaim peer write probe traverse Vitest's private temp root only for the duration of that probe, then restore its exact mode - leave production runtime behavior and non-guard command limits unchanged ## Evidence - main CI job 88920894839 returned `status: null` after the first guard child exceeded its 5-second `spawnSync` limit; the runner and later guard cases remained healthy - four prior main runs passed the same case in about 0.3 seconds, confirming a load-sensitive harness limit rather than a Hermes behavior regression - current-main Platform Vitest run 29919416442 passed the complete WSL suite (1,642 files / 19,227 tests) and then reproduced the isolated root-fixture failure - historical run 29307612216 reproduced that fixture failure before the OpenClaw upgrade and before #7379 - first exact-head WSL proof 29923652396 passed the complete WSL suite and both corrected Hermes root contracts; it then exposed the next root-only fixture defect - that config-reclaim test completed production normalization and all ownership/mode assertions; only its stepped-down write probe failed because PR #6690 made Vitest's shared temp ancestor private - current exact head `b7414d2f7f1bfd68028e9f43c11c1cc61033e266` includes current `main` at `f9924949922f8e554f94aeefdd23a993a801e7b4` - current exact-head platform proof: https://github.com/NVIDIA/NemoClaw/actions/runs/29929199550 - focused `nemoclaw-start-perms` suite: 16 passed, 3 Linux-root capability skips on macOS - `npm run build:cli` and `npm run check:diff`: passed - independent review of the narrow changes: no production findings Signed-off-by: Aaron Erickson <aerickson@nvidia.com> --------- Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Summary
Vitest runs now use one private, run-scoped temp root that is removed automatically after the run, including the process-exit fallback path. Test fixtures that previously bypassed the configured temp directory now use owned, unique paths and explicit cleanup. Set
NEMOCLAW_TEST_KEEP_TEMP=1to retain the run root for debugging.Changes
TMPDIR,TMP, andTEMPinto the run root, then restore the caller environment during teardown.Type of Change
Quality Gates
Verification
Verifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailablenpm run test:projects:checkpassed for 1,483 filesnpm testexercised 1,406 files; two aggregate-only failures passed in exact CI-mode isolation (10/10 and 64/64), so PR CI is the authoritative broad gatenpm run docsbuilds without warnings (doc changes only)Signed-off-by: Carlos Villela cvillela@nvidia.com
Summary by CodeRabbit
/tmppaths.