test(vitest): add owned resource fixtures - #6699
Merged
Merged
Conversation
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds shared test-resource and child-process ownership utilities, migrates CLI and broker tests to fixture-managed directories and processes, improves HOME cleanup, and adds proxy lifecycle termination coverage. ChangesTest resource ownership
Estimated code review effort: 4 (Complex) | ~60 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
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 |
Contributor
E2E Advisor RecommendationRequired E2E: None Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
|
Contributor
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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/helpers/owned-test-resources.ts (1)
93-98: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the suite’s POSIX
PATHcontract.Line 96 reintroduces
path.delimiter; update it, and the matching assertion intest/owned-test-resources.test.tsLine 42, to use:.Proposed fix
--- a/test/helpers/owned-test-resources.ts +++ b/test/helpers/owned-test-resources.ts - PATH: `${bin}${path.delimiter}${process.env.PATH ?? ""}`, + PATH: `${bin}:${process.env.PATH || ""}`, --- a/test/owned-test-resources.test.ts +++ b/test/owned-test-resources.test.ts -import path from "node:path"; - - expect(environment.PATH?.split(path.delimiter)[0]).toBe(testHome.bin); + expect(environment.PATH?.split(":")[0]).toBe(testHome.bin);Based on learnings, this test suite intentionally uses
:forPATHbecause its CI runners are POSIX-compatible.🤖 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/helpers/owned-test-resources.ts` around lines 93 - 98, Update the `environment` helper in `owned-test-resources.ts` to join `bin` and `process.env.PATH` with the literal `:` instead of `path.delimiter`, and update the matching assertion in `owned-test-resources.test.ts` to expect the same POSIX `PATH` format.Source: Learnings
🤖 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/ollama-auth-proxy-handler-helpers.ts`:
- Around line 90-134: Update the catch block around the readiness wait in the
proxy startup helper to preserve the original error: ensure cleanup always
attempts owner.terminate(), proxyOwners.delete(child) always executes, and any
termination failure is suppressed or otherwise prevented from replacing the
original error before rethrowing it.
---
Nitpick comments:
In `@test/helpers/owned-test-resources.ts`:
- Around line 93-98: Update the `environment` helper in
`owned-test-resources.ts` to join `bin` and `process.env.PATH` with the literal
`:` instead of `path.delimiter`, and update the matching assertion in
`owned-test-resources.test.ts` to expect the same POSIX `PATH` format.
🪄 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: cc91ce44-625a-4aa2-b677-1515eda6a084
📒 Files selected for processing (13)
test/cli/debug-command.test.tstest/cli/doctor-gateway-token.test.tstest/cli/helpers.test.tstest/cli/helpers.tstest/cli/logs.test.tstest/cli/sandbox-mutations.test.tstest/cli/snapshot-shields.test.tstest/helpers/child-process-lifecycle.tstest/helpers/owned-test-resources.tstest/hermes-tool-gateway-broker.test.tstest/ollama-auth-proxy-handler-helpers.tstest/ollama-auth-proxy-handler.test.tstest/owned-test-resources.test.ts
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/ollama-auth-proxy-handler-startup-cleanup.test.ts (1)
68-72: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer asserting retry behavior over owner call counts.
terminate(spawned)resolving already verifies the observable retry path. The exactownChildProcesscall counts couple this test to the current implementation and can fail after an equivalent ownership refactor. Assert child termination or another public outcome instead.As per path instructions, prefer observable outcomes through the public boundary over mock-call assertions.
🤖 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/ollama-auth-proxy-handler-startup-cleanup.test.ts` around lines 68 - 72, Remove the implementation-specific ownerMocks.ownChildProcess call-count assertion from the terminate(spawned) test. Keep assertions for the observable retry and termination outcomes, using failedTermination, retryTermination, or another public child-termination result rather than mock invocation counts.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/ollama-auth-proxy-handler-startup-cleanup.test.ts`:
- Around line 51-56: Remove the ad-hoc onTestFinished cleanup around spawned and
register the child process with the shared child owner instead, using the
existing ownership mechanism in this test setup. Ensure failed termination in
the mock closes the child before rejecting if needed, so cleanup remains
reliable without the added conditional branches or stdio-only checks.
---
Nitpick comments:
In `@test/ollama-auth-proxy-handler-startup-cleanup.test.ts`:
- Around line 68-72: Remove the implementation-specific
ownerMocks.ownChildProcess call-count assertion from the terminate(spawned)
test. Keep assertions for the observable retry and termination outcomes, using
failedTermination, retryTermination, or another public child-termination result
rather than mock invocation counts.
🪄 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: 883b1c66-cd26-4ed6-b0d2-a88c7134e2ab
📒 Files selected for processing (2)
test/ollama-auth-proxy-handler-helpers.tstest/ollama-auth-proxy-handler-startup-cleanup.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- test/ollama-auth-proxy-handler-helpers.ts
Comment on lines
+51
to
+56
| onTestFinished(async () => { | ||
| if (!spawned || spawned.stdio.every((stream) => stream?.destroyed !== false)) return; | ||
| const closed = once(spawned, "close"); | ||
| if (spawned.exitCode === null && spawned.signalCode === null) spawned.kill("SIGKILL"); | ||
| await closed; | ||
| }); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Use the shared child owner instead of ad-hoc cleanup.
This block causes test-conditionals:scan to fail because it adds the two if statements at Lines 52 and 54. It also checks only stdio closure; a child can close its streams while still running, leaving the proxy process leaked. Register the child with the shared owner, or make the mocked failed termination close it before rejecting, then remove this fallback.
As per coding guidelines, keep JavaScript/TypeScript function complexity low.
🤖 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/ollama-auth-proxy-handler-startup-cleanup.test.ts` around lines 51 - 56,
Remove the ad-hoc onTestFinished cleanup around spawned and register the child
process with the shared child owner instead, using the existing ownership
mechanism in this test setup. Ensure failed termination in the mock closes the
child before rejecting if needed, so cleanup remains reliable without the added
conditional branches or stdio-only checks.
Sources: Coding guidelines, Pipeline failures
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
21 tasks
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/ollama-auth-proxy-handler-helpers.ts (2)
82-129: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winHandle
spawn()failures on the proxy child.ownChildProcess()doesn’t attach an"error"listener, soENOENT/permission failures can become an unhandled event instead of rejecting this startup path. Addchild.once("error", reject)next to the existing exit/readiness handling.🤖 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/ollama-auth-proxy-handler-helpers.ts` around lines 82 - 129, Update the startup promise in the proxy child flow around `ownChildProcess` and the existing `child.once("exit", ...)` handler to attach a one-time `child.once("error", reject)` listener. Ensure spawn failures such as ENOENT or permission errors reject the readiness promise and follow the existing cleanup path.
95-128: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winDestroy the readiness probe on settle
The timeout and early-exit paths reject without aborting the activehttp.ClientRequest. If the probe connects and then stalls, the socket can stay open afterstartProxyhas already failed. Track the current request anddestroy()it when settling on timeout or child exit.🤖 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/ollama-auth-proxy-handler-helpers.ts` around lines 95 - 128, Update the readiness logic in the proxy startup helper to track the active http.ClientRequest created by tryConnect. When the timeout or child exit settles the promise, destroy the current request before rejecting, while preserving the existing retry and successful-response behavior.
🧹 Nitpick comments (1)
test/ollama-auth-proxy-handler-helpers.ts (1)
76-139: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the readiness-polling block to reduce
startProxycomplexity.The readiness Promise (timer, recursive
tryConnect, request callback, error retry, exit handler) makesstartProxya large multi-branch function. Extracting it into a dedicatedwaitForReadiness(child, port, timeoutMs)helper would isolate the polling state machine and keepstartProxyfocused on spawn/ownership/cleanup.async function waitForReadiness(child: ChildProcess, port: number, timeoutMs: number): Promise<void> { await new Promise<void>((resolve, reject) => { // ... existing polling logic moved here }); }As per coding guidelines,
**/*.{js,ts}should "Keep function complexity low", which this extraction addresses.🤖 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/ollama-auth-proxy-handler-helpers.ts` around lines 76 - 139, Extract the readiness Promise and its timer, recursive tryConnect polling, request handling, and child exit handling from startProxy into a dedicated waitForReadiness(child, port, timeoutMs) helper. Update startProxy to call this helper with the configured readiness port and timeout while preserving the existing startup, timeout, retry, and early-exit behavior.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@test/ollama-auth-proxy-handler-helpers.ts`:
- Around line 82-129: Update the startup promise in the proxy child flow around
`ownChildProcess` and the existing `child.once("exit", ...)` handler to attach a
one-time `child.once("error", reject)` listener. Ensure spawn failures such as
ENOENT or permission errors reject the readiness promise and follow the existing
cleanup path.
- Around line 95-128: Update the readiness logic in the proxy startup helper to
track the active http.ClientRequest created by tryConnect. When the timeout or
child exit settles the promise, destroy the current request before rejecting,
while preserving the existing retry and successful-response behavior.
---
Nitpick comments:
In `@test/ollama-auth-proxy-handler-helpers.ts`:
- Around line 76-139: Extract the readiness Promise and its timer, recursive
tryConnect polling, request handling, and child exit handling from startProxy
into a dedicated waitForReadiness(child, port, timeoutMs) helper. Update
startProxy to call this helper with the configured readiness port and timeout
while preserving the existing startup, timeout, retry, and early-exit behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 094c7e98-1f1f-4abe-93cc-a14edac61d21
📒 Files selected for processing (2)
test/ollama-auth-proxy-handler-helpers.tstest/ollama-auth-proxy-handler-startup-cleanup.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- test/ollama-auth-proxy-handler-startup-cleanup.test.ts
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Collaborator
Author
|
Addressed the latest CodeRabbit readiness findings in signed commit 752dee2: spawn errors now reject through the existing owner cleanup path, timeout/exit settlement destroys the active HTTP request and clears retry state/listeners, and the polling state machine is extracted into |
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/ollama-auth-proxy-handler-helpers.ts (1)
172-177: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
terminate()can leave a staleWeakMapentry ifowner.terminate()throws.Unlike
startProxy's catch block (Lines 160-169), which now wrapsowner.terminate()in try/catch/finally soproxyOwners.delete(child)always runs,terminate()here deletes the map entry only on the happy path. Ifowner.terminate()throws (the same failure mode already identified forownChildProcessin the earlier review — e.g. the child not closing after SIGKILL),proxyOwners.delete(child)is skipped and a laterterminate(child)call on the same child will reuse the same (possibly broken) owner instead of starting fresh.🐛 Proposed fix to ensure cleanup runs regardless of termination outcome
export async function terminate(child: ChildProcess | undefined): Promise<void> { if (!child) return; const owner = proxyOwners.get(child) ?? ownChildProcess(child); - await owner.terminate(); - proxyOwners.delete(child); + try { + await owner.terminate(); + } finally { + proxyOwners.delete(child); + } }🤖 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/ollama-auth-proxy-handler-helpers.ts` around lines 172 - 177, Update terminate() to ensure proxyOwners.delete(child) runs in a finally block around owner.terminate(), while preserving the existing error propagation behavior. This guarantees the WeakMap entry is removed whether termination succeeds or throws.
🧹 Nitpick comments (1)
test/ollama-auth-proxy-handler.test.ts (1)
153-174: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThese two tests lean on mock-call/listener-count assertions rather than observable outcomes.
toHaveBeenCalledOnce()on the mockedend/destroyandchild.listenerCount(...)checks assert onwaitForProxyReadiness's internal implementation surface rather than a purely public-boundary outcome. Per path instructions, observable outcomes are preferred over mock-call/private-shape assertions. That said, given the difficulty of deterministically simulating a stalled real HTTP connection or listener-leak without mocking, this is a reasonable pragmatic trade-off for covering the cleanup contract added in this PR — flagging for awareness rather than requesting an immediate rewrite.Based on path instructions ("Prefer observable outcomes through the public boundary over source-text, private-shape, or mock-call assertions") for
**/*.test.{ts,js,mts,mjs,cts,cjs}.Also applies to: 176-186
🤖 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/ollama-auth-proxy-handler.test.ts` around lines 153 - 174, No code change is required; the cleanup-contract tests around waitForProxyReadiness intentionally use mocked request calls and child listener counts because deterministic public-boundary verification is impractical. Retain the existing assertions unless a reliable real-connection test can validate the same stalled-request cleanup behavior without depending on internal implementation details.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.
Outside diff comments:
In `@test/ollama-auth-proxy-handler-helpers.ts`:
- Around line 172-177: Update terminate() to ensure proxyOwners.delete(child)
runs in a finally block around owner.terminate(), while preserving the existing
error propagation behavior. This guarantees the WeakMap entry is removed whether
termination succeeds or throws.
---
Nitpick comments:
In `@test/ollama-auth-proxy-handler.test.ts`:
- Around line 153-174: No code change is required; the cleanup-contract tests
around waitForProxyReadiness intentionally use mocked request calls and child
listener counts because deterministic public-boundary verification is
impractical. Retain the existing assertions unless a reliable real-connection
test can validate the same stalled-request cleanup behavior without depending on
internal implementation details.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b2c7a6fb-96c9-4f02-8f58-da0741e5b9ac
📒 Files selected for processing (3)
test/ollama-auth-proxy-handler-helpers.tstest/ollama-auth-proxy-handler-startup-cleanup.test.tstest/ollama-auth-proxy-handler.test.ts
💤 Files with no reviewable changes (1)
- test/ollama-auth-proxy-handler-startup-cleanup.test.ts
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Collaborator
Author
|
Addressed the stale-owner retry finding in signed commit 4b6f585. |
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
cv
added a commit
that referenced
this pull request
Jul 12, 2026
<!-- markdownlint-disable MD041 --> ## Summary Deterministic Vitest projects now restore spies, clear mock call state, and undo stubbed environment variables and globals before every test while leaving live E2E projects unchanged. This is PR 5 in the #6692 stack and is based on #6699. ## Related Issue Part of #6692 ## Changes - Enable `clearMocks`, `restoreMocks`, `unstubEnvs`, and `unstubGlobals` in the `cli`, `integration`, `installer-integration`, `package-contract`, `plugin`, and `e2e-support` projects without enabling `mockReset`. - Keep the two live E2E projects outside automatic cleanup and add a contract that guards both project-local settings and inherited root settings. - Move the exposed `Date.now` and `console.log` spies into per-test setup, and make the non-SSH dashboard test independent of the developer's ambient SSH environment. - Share the four cleanup settings across the five root-owned deterministic projects. The plugin keeps explicit settings to preserve its package root; `test/vitest-state-isolation.test.ts` protects parity and live exclusions. - Document the test-state ownership contract for contributors, including the narrow import-time stub exception. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [x] 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: user-facing NemoClaw behavior is unchanged; the contributor test contract is documented in `AGENTS.md` and `CONTRIBUTING.md`. - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: independent final review found no findings; only onboarding test files changed, not production onboarding behavior. - [ ] 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 — `68` targeted tests passed across CLI, plugin, and integration projects; plugin and CLI typechecks passed; the dashboard test also passed with an ambient `SSH_CONNECTION`. - [x] Applicable broad gate passed — `npm test`: 1,440 files and 16,461 tests passed; 40 expected opt-in/platform tests skipped. - [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) — passed with zero errors; Fern reported two unrelated baseline warnings for unauthenticated redirect checking and existing light-mode contrast. - [ ] 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> --------- Signed-off-by: Carlos Villela <cvillela@nvidia.com>
21 tasks
cv
added a commit
that referenced
this pull request
Jul 12, 2026
## Summary Vitest project validation now compares every filesystem test candidate with Vitest's complete eight-project roster, rejecting omissions, overlaps, wrong routing, unexpected files, and unexpected project names. The shared credential-free E2E workflow now applies Vitest's native e2e/credential-free tag filter in addition to the existing safe matrix scanner. The dashboard-port regression fixture also uses a coherent fake OpenShell 0.0.72 component set and an OS-assigned gateway port so the repo-wide gate stays hermetic. This is the final entry in the ordered #6692 stack after #6693, #6696, #6697, #6699, #6700, #6701, #6702, and #6705 landed. ## Related Issue Closes #6692 ## Changes - Discover broad test/spec JavaScript and TypeScript candidates under src, test, and nemoclaw/src, including nested dist, coverage, and .venv paths that Vitest would see. - Compare candidate routing with Vitest file listings and the complete roster from list-tags JSON, including empty and opt-in projects. - Produce sorted diagnostics for zero membership, overlap, wrong project, unsupported candidates, unexpected listed files, and missing or extra projects. - Invoke Vitest portably through process.execPath and node_modules/vitest/vitest.mjs. - Require the exact native credential-free tag filter in the shared E2E workflow contract while retaining the TypeScript scanner's path, project, ID, and declaration checks. - Keep the dashboard-port exhaustion regression independent of port 8080, developer OpenShell overrides, channel selection, and network installation. ## 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: these are contributor/test-harness safeguards; AGENTS.md now states the exhaustive project-membership contract, the E2E README already documents the native module tag, and the required documentation-writer review found no user-facing change. - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: independent review covered the shared credential-free workflow and onboarding regression fixture; it confirmed the filter preserves the existing scanner and the fixture's child-only environment cannot reach a real OpenShell install. - [ ] 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 — 46 integration tests passed across the boundary and dashboard-port files; 4 E2E-support workflow tests passed; project membership is exact for 1,537 candidates across 8 projects; CLI type-check, title style, Biome, and diff checks passed. The fixture also passed with hostile inherited OpenShell paths and dev-channel selection. - [x] Applicable broad gate passed — npm test passed 1,456 files / 16,515 tests before the final focused hardening; npm run check passed the repo-wide structural, CLI coverage, and plugin coverage gates, followed by focused reruns and normal hooks after the final child-environment pin. - [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 (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** - Credential-free end-to-end test runs now execute only tests matching the credential-free tag. - Vitest project validation now checks for missing, overlapping, unsupported, wrong, zero-membership, and unexpected test-to-project assignments, plus roster mismatches. - Expanded boundary tests cover test candidate discovery, expected project mapping, Vitest invocation resolution, and listing/roster parsing. - Improved onboarding “dashboard port exhaustion” assertions for more hermetic, reliable verification. - **Documentation** - Updated testing guidance to require Vitest project globs be disjoint and exhaustive, enforced by the projects check. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Hadar301
pushed a commit
to Hadar301/NemoClaw-OpenShift
that referenced
this pull request
Jul 12, 2026
## Summary Add per-test ownership for temporary HOME/bin directories, loopback servers, and child processes so cleanup is awaited even when setup or assertions fail. Migrate the current high-use CLI helpers and high-risk Hermes broker and Ollama proxy consumers, including cleanup of each implicit CLI HOME. This is PR4 in the NVIDIA#6692 stack; PRs NVIDIA#6696 and NVIDIA#6697 are already merged. ## Related Issue Related to NVIDIA#6692 ## Changes - Add `OwnedTestResources` and focused `test.extend` fixtures for LIFO cleanup of temporary directories, HOME/bin environments, servers, and child processes. - Add a shared child-process owner that waits for `close`, attempts graceful termination, escalates to `SIGKILL` when required, and confirms piped stdio is closed. The owned-resource fixture and Ollama proxy harness share this lifecycle because suite teardown did not consistently cover setup failures or await process closure. - Track server lifetime from ownership registration so fixture cleanup waits for both fixture-initiated shutdown and shutdown already started by code under test. - Remove the unowned per-invocation CLI HOME by deleting it when `runWithEnvInternal` completes, while preserving explicit caller-owned HOME directories. - Migrate the current repeated CLI HOME/bin setup helpers and the high-risk logs, doctor, Hermes broker, and Ollama proxy process/server consumers. - Add focused regressions for idempotent cleanup, server shutdown already in progress, graceful and forced child termination, proxy readiness failure cleanup, and CLI HOME cleanup after success, failure, and timeout. ## 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: This changes test-only resource ownership and cleanup. The required documentation-writer review confirmed there is no user-facing behavior, test command, lane, or contributor contract to document. - [ ] 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 — command/result or justification: - `npx vitest run --project integration test/owned-test-resources.test.ts test/cli/helpers.test.ts test/cli/logs.test.ts test/cli/debug-command.test.ts test/cli/doctor-gateway-token.test.ts test/cli/sandbox-mutations.test.ts test/cli/snapshot-shields.test.ts test/hermes-tool-gateway-broker.test.ts test/ollama-auth-proxy-handler.test.ts` — 9 files, 71/71 tests passed on the rebased branch. - `NEMOCLAW_TEST_TIMEOUT=60000 npx vitest run --project integration test/cli/helpers.test.ts` — 12/12 tests passed; the synchronous timeout regression remained fixed at 2 seconds. - `npx vitest run --project integration test/cli/logs.test.ts --detectAsyncLeaks` — 10/10 tests passed with no async leaks. - `npm run typecheck:cli`, Biome on all changed files, `npm run source-shape:check`, `npm run test-size:check`, `npm run test:projects:check`, `npm run test:imports:check`, and `npm run test:titles:check` passed. - [x] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: `npm test` passed on the rebased branch (1,438 files and 16,457 tests passed; 3 files and 40 opt-in platform tests skipped). - [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 CLI test hermeticity by standardizing on `owned-test-resources` with resource-scoped setup/teardown across logs, doctor, debug, sandbox mutations, snapshot shields, and sandbox/server-related suites. * Expanded HOME-related coverage, verifying no implicit HOME is left behind for successful, failed, and timed-out runs (while honoring explicit HOME). * Added dedicated child-process lifecycle ownership utilities and new verification tests for graceful shutdown, SIGTERM→SIGKILL escalation, and reliable stream closure/cleanup. * Strengthened ollama auth proxy startup/cleanup tests, including readiness timeout failures, listener removal, and retryable cleanup behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Hadar301
pushed a commit
to Hadar301/NemoClaw-OpenShift
that referenced
this pull request
Jul 12, 2026
<!-- markdownlint-disable MD041 --> ## Summary Deterministic Vitest projects now restore spies, clear mock call state, and undo stubbed environment variables and globals before every test while leaving live E2E projects unchanged. This is PR 5 in the NVIDIA#6692 stack and is based on NVIDIA#6699. ## Related Issue Part of NVIDIA#6692 ## Changes - Enable `clearMocks`, `restoreMocks`, `unstubEnvs`, and `unstubGlobals` in the `cli`, `integration`, `installer-integration`, `package-contract`, `plugin`, and `e2e-support` projects without enabling `mockReset`. - Keep the two live E2E projects outside automatic cleanup and add a contract that guards both project-local settings and inherited root settings. - Move the exposed `Date.now` and `console.log` spies into per-test setup, and make the non-SSH dashboard test independent of the developer's ambient SSH environment. - Share the four cleanup settings across the five root-owned deterministic projects. The plugin keeps explicit settings to preserve its package root; `test/vitest-state-isolation.test.ts` protects parity and live exclusions. - Document the test-state ownership contract for contributors, including the narrow import-time stub exception. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [x] 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: user-facing NemoClaw behavior is unchanged; the contributor test contract is documented in `AGENTS.md` and `CONTRIBUTING.md`. - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: independent final review found no findings; only onboarding test files changed, not production onboarding behavior. - [ ] 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 — `68` targeted tests passed across CLI, plugin, and integration projects; plugin and CLI typechecks passed; the dashboard test also passed with an ambient `SSH_CONNECTION`. - [x] Applicable broad gate passed — `npm test`: 1,440 files and 16,461 tests passed; 40 expected opt-in/platform tests skipped. - [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) — passed with zero errors; Fern reported two unrelated baseline warnings for unauthenticated redirect checking and existing light-mode contrast. - [ ] 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> --------- Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Hadar301
pushed a commit
to Hadar301/NemoClaw-OpenShift
that referenced
this pull request
Jul 12, 2026
…A#6707) ## Summary Vitest project validation now compares every filesystem test candidate with Vitest's complete eight-project roster, rejecting omissions, overlaps, wrong routing, unexpected files, and unexpected project names. The shared credential-free E2E workflow now applies Vitest's native e2e/credential-free tag filter in addition to the existing safe matrix scanner. The dashboard-port regression fixture also uses a coherent fake OpenShell 0.0.72 component set and an OS-assigned gateway port so the repo-wide gate stays hermetic. This is the final entry in the ordered NVIDIA#6692 stack after NVIDIA#6693, NVIDIA#6696, NVIDIA#6697, NVIDIA#6699, NVIDIA#6700, NVIDIA#6701, NVIDIA#6702, and NVIDIA#6705 landed. ## Related Issue Closes NVIDIA#6692 ## Changes - Discover broad test/spec JavaScript and TypeScript candidates under src, test, and nemoclaw/src, including nested dist, coverage, and .venv paths that Vitest would see. - Compare candidate routing with Vitest file listings and the complete roster from list-tags JSON, including empty and opt-in projects. - Produce sorted diagnostics for zero membership, overlap, wrong project, unsupported candidates, unexpected listed files, and missing or extra projects. - Invoke Vitest portably through process.execPath and node_modules/vitest/vitest.mjs. - Require the exact native credential-free tag filter in the shared E2E workflow contract while retaining the TypeScript scanner's path, project, ID, and declaration checks. - Keep the dashboard-port exhaustion regression independent of port 8080, developer OpenShell overrides, channel selection, and network installation. ## 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: these are contributor/test-harness safeguards; AGENTS.md now states the exhaustive project-membership contract, the E2E README already documents the native module tag, and the required documentation-writer review found no user-facing change. - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: independent review covered the shared credential-free workflow and onboarding regression fixture; it confirmed the filter preserves the existing scanner and the fixture's child-only environment cannot reach a real OpenShell install. - [ ] 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 — 46 integration tests passed across the boundary and dashboard-port files; 4 E2E-support workflow tests passed; project membership is exact for 1,537 candidates across 8 projects; CLI type-check, title style, Biome, and diff checks passed. The fixture also passed with hostile inherited OpenShell paths and dev-channel selection. - [x] Applicable broad gate passed — npm test passed 1,456 files / 16,515 tests before the final focused hardening; npm run check passed the repo-wide structural, CLI coverage, and plugin coverage gates, followed by focused reruns and normal hooks after the final child-environment pin. - [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 (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** - Credential-free end-to-end test runs now execute only tests matching the credential-free tag. - Vitest project validation now checks for missing, overlapping, unsupported, wrong, zero-membership, and unexpected test-to-project assignments, plus roster mismatches. - Expanded boundary tests cover test candidate discovery, expected project mapping, Vitest invocation resolution, and listing/roster parsing. - Improved onboarding “dashboard port exhaustion” assertions for more hermetic, reliable verification. - **Documentation** - Updated testing guidance to require Vitest project globs be disjoint and exhaustive, enforced by the projects check. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Carlos Villela <cvillela@nvidia.com>
wscurran
added
the
area: ci
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Add per-test ownership for temporary HOME/bin directories, loopback servers, and child processes so cleanup is awaited even when setup or assertions fail. Migrate the current high-use CLI helpers and high-risk Hermes broker and Ollama proxy consumers, including cleanup of each implicit CLI HOME. This is PR4 in the #6692 stack; PRs #6696 and #6697 are already merged.
Related Issue
Related to #6692
Changes
OwnedTestResourcesand focusedtest.extendfixtures for LIFO cleanup of temporary directories, HOME/bin environments, servers, and child processes.close, attempts graceful termination, escalates toSIGKILLwhen required, and confirms piped stdio is closed. The owned-resource fixture and Ollama proxy harness share this lifecycle because suite teardown did not consistently cover setup failures or await process closure.runWithEnvInternalcompletes, while preserving explicit caller-owned HOME directories.Type of Change
Quality Gates
Verification
Verifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailablenpx vitest run --project integration test/owned-test-resources.test.ts test/cli/helpers.test.ts test/cli/logs.test.ts test/cli/debug-command.test.ts test/cli/doctor-gateway-token.test.ts test/cli/sandbox-mutations.test.ts test/cli/snapshot-shields.test.ts test/hermes-tool-gateway-broker.test.ts test/ollama-auth-proxy-handler.test.ts— 9 files, 71/71 tests passed on the rebased branch.NEMOCLAW_TEST_TIMEOUT=60000 npx vitest run --project integration test/cli/helpers.test.ts— 12/12 tests passed; the synchronous timeout regression remained fixed at 2 seconds.npx vitest run --project integration test/cli/logs.test.ts --detectAsyncLeaks— 10/10 tests passed with no async leaks.npm run typecheck:cli, Biome on all changed files,npm run source-shape:check,npm run test-size:check,npm run test:projects:check,npm run test:imports:check, andnpm run test:titles:checkpassed.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result:npm testpassed on the rebased branch (1,438 files and 16,457 tests passed; 3 files and 40 opt-in platform tests skipped).npm run docsbuilds without warnings (doc changes only)Signed-off-by: Carlos Villela cvillela@nvidia.com
Summary by CodeRabbit
owned-test-resourceswith resource-scoped setup/teardown across logs, doctor, debug, sandbox mutations, snapshot shields, and sandbox/server-related suites.