perf(test): run MCP lifecycle checks in process - #6373
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughFour MCP-related test files now run in-process under Vitest instead of spawning child processes. The tests use hoisted mocks, direct bridge/probe calls, and shared mock state to assert lifecycle, capability, startup, and destroy behaviors. ChangesTest Harness Migration
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in the Show a code coverage summary of the most covered files.
TypeScript / code-coverage/cliThe overall coverage in the Show a code coverage summary of the most covered files.
Updated |
Contributor
E2E Advisor RecommendationRequired E2E: None Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
|
Contributor
E2E Target RecommendationRequired E2E targets: None Full E2E target advisor summaryE2E Target AdvisorBase: Required E2E targets
Optional E2E targets
Relevant changed files
|
Contributor
PR Review Advisor (Nemotron Ultra) — No blocking findingsMerge posture: No blocking advisor findings Action checklist
Findings index
Review findings by urgency: 0 required fixes, 0 items to resolve/justify, 4 in-scope improvements
|
Contributor
PR Review Advisor — No blocking findingsMerge posture: No blocking advisor findings Action checklist
Test follow-ups to resolve or justifyIf these cover changed behavior, prefer adding them in this PR; otherwise state why existing coverage is enough or link the follow-up.
This is an automated, non-binding review; it still expects maintainers and agents to respond to each required or warning item. Treat suggestions as current-PR improvements when they touch changed code; defer only with maintainer rationale or a linked follow-up. A human maintainer must make the final merge decision. |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (5)
test/deepagents-mcp-legacy-lifecycle.test.ts (1)
161-214: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReduce script-fragment coupling in the legacy Deep Agents lifecycle mock.
These mocks still key off literal snippets from the generated teardown/probe scripts, so harmless refactors can make the test silently fall through. The ownership-matching path already has direct coverage elsewhere, so this suite can focus on lifecycle behavior instead of re-encoding script internals.🤖 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/deepagents-mcp-legacy-lifecycle.test.ts` around lines 161 - 214, The legacy Deep Agents lifecycle mocks are too tightly coupled to exact script fragments in executeSandboxCommand and executeSandboxExecCommand, which makes the test brittle to harmless script refactors. Update the mock branching in this test to assert lifecycle outcomes through stable signals from the lifecycle flow instead of matching literal teardown/probe snippets, using the executeSandboxCommand and executeSandboxExecCommand handlers as the main touchpoints. Keep the existing ownership-matching coverage elsewhere and simplify this suite to validate registration/removal/rollback behavior without depending on embedded script internals.Source: Path instructions
test/mcp-destroy-lifecycle.test.ts (4)
277-277: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTest titles lack the required
(#1234)issue-ref suffix.None of the
it(...)titles in this file (e.g.`clears a providerless preflighted add during ${method}`,"prepares an absent-sandbox rebuild without adapter exec or provider detach") end with a local issue reference in a(#1234)suffix.As per coding guidelines, "Root-level integration tests under
test/should import source code, use ESM imports, and use behavior-oriented titles with local issue refs in a final(#1234)suffix."Also applies to: 306-306, 329-329, 351-351, 368-368, 393-393, 416-416, 449-449, 475-475, 506-506, 529-529, 557-557, 590-590, 628-628, 660-660
🤖 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/mcp-destroy-lifecycle.test.ts` at line 277, Update the affected `it(...)` titles in `mcp-destroy-lifecycle.test.ts` so each behavior-oriented test name ends with the required local issue reference suffix in the form `(`#1234`)`. Keep the existing intent of titles like `clears a providerless preflighted add during ${method}` and the other listed cases, but append the suffix consistently across all matching tests in this file.Source: Coding guidelines
660-693: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHand-built preparation object bypasses
prepareMcpBridgesForDestroy/prepareMcpBridgesForRebuild.This test constructs the
preparationobject literal directly (Lines 674-680) instead of deriving it from an actual prepare call. It couples the test to the internal shape of the preparation type and doesn't verify that a realprepareMcpBridgesForDestroy/rebuild call would ever produce this specific combination of fields (e.g.,destroyAlreadyPending: truewith emptydetachedProviderEntries).🤖 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/mcp-destroy-lifecycle.test.ts` around lines 660 - 693, The test is relying on a manually constructed preparation object instead of validating the real output from the preparation flow. Update the “does not let force delete a drifted global provider” test to build `preparation` by calling the actual `prepareMcpBridgesForDestroy` or `prepareMcpBridgesForRebuild` path used by `finalizeMcpBridgesAfterSandboxDelete`, and then assert the same forced-delete behavior on that returned object. Keep the assertions on `finalizeMcpBridgesAfterSandboxDelete`, but avoid hardcoding internal `preparation` fields so the test stays aligned with the real contract.
157-213: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftMocks reimplement production CLI/credential-resolution logic rather than stubbing it.
runOpenshellProviderCommand.mockImplementation(Lines 157-213) re-derives provider/attach/detach/delete state transitions and status codes that mirror the real OpenShell CLI's behavior, andexecuteSandboxExecCommand.mockImplementation(Lines 239-264) decodes base64 "proof" scripts and re-implements the credential-resolution/observation logic that production code (buildMcpCredentialRevisionObservationCommand) generates. Because both sides of the test now encode the same assumptions about command shape and decoding, a bug or format drift in the production command-builders could go undetected — the mock will "agree" with whatever it's told to check, rather than exercising the real behavior that the previous subprocess-based tests exercised end-to-end.Consider trimming these mocks to the minimal surface needed per test (return canned responses per scenario) rather than a full state-machine reimplementation, or add a companion contract test that exercises the actual command-builder output against this mock's parsing to catch drift.
As per path instructions, "Flag copied production algorithms, broad mocks that bypass the behavior under test, and conditionals that make a test pass without exercising its claim."
Also applies to: 239-264
🤖 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/mcp-destroy-lifecycle.test.ts` around lines 157 - 213, The test mocks are reimplementing production CLI and credential-resolution behavior instead of stubbing outcomes, which can hide drift in the real command builders. Simplify runOpenshellProviderCommand and executeSandboxExecCommand to return canned responses per scenario and avoid state-machine/base64 parsing logic; keep only the minimal branching needed for each test case. If you need coverage for the command shape, add a separate contract test around buildMcpCredentialRevisionObservationCommand rather than duplicating production algorithms in the mock implementations.Source: Path instructions
67-67: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove unused OpenShell fixture setup Remove
MATCHING_OPENSHELLand theprocess.env.NEMOCLAW_OPENSHELL_BINassignment here; this test fully mocksrunOpenshellProviderCommand, so the fixture binary never gets used.🤖 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/mcp-destroy-lifecycle.test.ts` at line 67, Remove the unused OpenShell fixture setup in this test: the MATCHING_OPENSHELL constant and the process.env.NEMOCLAW_OPENSHELL_BIN assignment are unnecessary because mcp-destroy-lifecycle.test.ts fully mocks runOpenshellProviderCommand. Clean up the test setup so it no longer resolves the fixture binary or mutates the env var, and keep the mock-based lifecycle assertions intact.
🤖 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.
Nitpick comments:
In `@test/deepagents-mcp-legacy-lifecycle.test.ts`:
- Around line 161-214: The legacy Deep Agents lifecycle mocks are too tightly
coupled to exact script fragments in executeSandboxCommand and
executeSandboxExecCommand, which makes the test brittle to harmless script
refactors. Update the mock branching in this test to assert lifecycle outcomes
through stable signals from the lifecycle flow instead of matching literal
teardown/probe snippets, using the executeSandboxCommand and
executeSandboxExecCommand handlers as the main touchpoints. Keep the existing
ownership-matching coverage elsewhere and simplify this suite to validate
registration/removal/rollback behavior without depending on embedded script
internals.
In `@test/mcp-destroy-lifecycle.test.ts`:
- Line 277: Update the affected `it(...)` titles in
`mcp-destroy-lifecycle.test.ts` so each behavior-oriented test name ends with
the required local issue reference suffix in the form `(`#1234`)`. Keep the
existing intent of titles like `clears a providerless preflighted add during
${method}` and the other listed cases, but append the suffix consistently across
all matching tests in this file.
- Around line 660-693: The test is relying on a manually constructed preparation
object instead of validating the real output from the preparation flow. Update
the “does not let force delete a drifted global provider” test to build
`preparation` by calling the actual `prepareMcpBridgesForDestroy` or
`prepareMcpBridgesForRebuild` path used by
`finalizeMcpBridgesAfterSandboxDelete`, and then assert the same forced-delete
behavior on that returned object. Keep the assertions on
`finalizeMcpBridgesAfterSandboxDelete`, but avoid hardcoding internal
`preparation` fields so the test stays aligned with the real contract.
- Around line 157-213: The test mocks are reimplementing production CLI and
credential-resolution behavior instead of stubbing outcomes, which can hide
drift in the real command builders. Simplify runOpenshellProviderCommand and
executeSandboxExecCommand to return canned responses per scenario and avoid
state-machine/base64 parsing logic; keep only the minimal branching needed for
each test case. If you need coverage for the command shape, add a separate
contract test around buildMcpCredentialRevisionObservationCommand rather than
duplicating production algorithms in the mock implementations.
- Line 67: Remove the unused OpenShell fixture setup in this test: the
MATCHING_OPENSHELL constant and the process.env.NEMOCLAW_OPENSHELL_BIN
assignment are unnecessary because mcp-destroy-lifecycle.test.ts fully mocks
runOpenshellProviderCommand. Clean up the test setup so it no longer resolves
the fixture binary or mutates the env var, and keep the mock-based lifecycle
assertions intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f7f65c28-0d95-4c63-9bca-072c85898ea5
📒 Files selected for processing (4)
test/deepagents-mcp-legacy-lifecycle.test.tstest/deepagents-mcp-runtime-capability.test.tstest/hermes-mcp-startup-probe.test.tstest/mcp-destroy-lifecycle.test.ts
| adapterCalls.push(command); | ||
| switch (true) { | ||
| case command === "/usr/local/bin/deepagents-code --nemoclaw-mcp-capability": | ||
| return deepAgentsCapability |
| adapterCalls.push(command); | ||
| switch (true) { | ||
| case command === "/usr/local/bin/deepagents-code --nemoclaw-mcp-capability": | ||
| return deepAgentsCapability |
Collaborator
Author
|
Advisor follow-up rationale:
|
wscurran
added
area: ci
Hadar301
pushed a commit
to Hadar301/NemoClaw-OpenShift
that referenced
this pull request
Jul 12, 2026
<!-- markdownlint-disable MD041 --> ## Summary Run four MCP lifecycle and runtime-capability suites directly in Vitest instead of cold-starting a Node process for every scenario. The focused local run falls from 14.25 seconds to 2.97 seconds wall-clock while preserving all 52 behaviors, real registry filesystem state, and the two real OpenShell fixture version probes. ## Related Issue Contributes to NVIDIA#6245. ## Changes - Replace 55 outer Node isolation children across the destroy, Hermes startup, and Deep Agents lifecycle/capability suites with direct source calls and hoisted Vitest mocks. - Preserve per-scenario provider, attachment, adapter, policy, recovery, and environment isolation. - Keep real temporary registry and lifecycle-lock filesystem behavior plus the two nested OpenShell fixture version checks. - Leave production modules and genuine crash/process-contract tests unchanged. ## 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 <!-- Check exactly one tests line and one docs line. Check other lines when applicable. Add every requested justification or approval reference. --> - [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 only changes internal test isolation mechanics; MCP commands, configuration, registry semantics, and runtime behavior are unchanged. - [ ] 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 <!-- Check each applicable item only when supported by the requested evidence. Run targeted tests once per relevant change set and rerun after later edits or hook autofixes that can affect the tested behavior. Do not rerun hook-covered checks. --> - [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/mcp-destroy-lifecycle.test.ts test/hermes-mcp-startup-probe.test.ts test/deepagents-mcp-legacy-lifecycle.test.ts test/deepagents-mcp-runtime-capability.test.ts` (4 files, 52/52 tests passed; 2.65s Vitest / 2.97s wall-clock); `npm run test-conditionals:scan -- --top 25` (completed; changed files contain zero `if` statements). - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: - [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) --- <!-- DCO sign-off is required in this PR description, and every commit must appear as Verified in GitHub. Run: git config user.name && git config user.email --> Signed-off-by: Carlos Villela <cvillela@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Improved coverage for lifecycle, startup, and runtime capability-probe flows. * Reworked multiple suites to run in-process with mocked dependencies for faster, more reliable execution. * Expanded assertions around destroy/rebuild/restore behaviors, including rollback/reattach paths and durable-marker handling. * Strengthened validation of cleanup and state transitions, and improved error-message checking to better prevent regressions. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
22 tasks
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
3 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
Run four MCP lifecycle and runtime-capability suites directly in Vitest instead of cold-starting a Node process for every scenario. The focused local run falls from 14.25 seconds to 2.97 seconds wall-clock while preserving all 52 behaviors, real registry filesystem state, and the two real OpenShell fixture version probes.
Related Issue
Contributes to #6245.
Changes
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/mcp-destroy-lifecycle.test.ts test/hermes-mcp-startup-probe.test.ts test/deepagents-mcp-legacy-lifecycle.test.ts test/deepagents-mcp-runtime-capability.test.ts(4 files, 52/52 tests passed; 2.65s Vitest / 2.97s wall-clock);npm run test-conditionals:scan -- --top 25(completed; changed files contain zeroifstatements).npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result:npm run docsbuilds without warnings (doc changes only)Signed-off-by: Carlos Villela cvillela@nvidia.com
Summary by CodeRabbit