Repository navigation
test(e2e): preserve MCP discovery diagnostics - #8751
Conversation
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
📝 WalkthroughWalkthroughAuthenticated MCP discovery now produces redacted JSON diagnostics after retries. Trusted-private and provider-specific tests pass artifact writers. Support tests validate diagnostic metadata, filenames, cleanup, rejection behavior, and secret exclusion. ChangesMCP discovery diagnostics
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
senthilr-nv
added
chore
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/e2e/live/mcp-bridge-tool-discovery.ts`:
- Around line 62-66: Sanitize the status projection used by the live diagnostics
artifact instead of copying unknown values from status fields; update the
projection around provider, policy, adapter, trustedPrivateTarget, and
toolDiscovery in test/e2e/live/mcp-bridge-tool-discovery.ts to include only
explicit secret-safe, non-sensitive values and exclude credential-resolution,
credentials, tokens, sessions, request payloads, and arbitrary environment data.
In test/e2e/support/mcp-bridge-tool-discovery.test.ts, use a real ArtifactSink,
write and read the artifact, seed sensitive values in raw status fields, and
assert those values are absent.
🪄 Autofix
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: 3ba3041b-6f80-4f1b-b1bf-4893fb19ded8
📒 Files selected for processing (4)
test/e2e/live/mcp-bridge-tool-discovery.tstest/e2e/live/mcp-bridge-trusted-private.tstest/e2e/live/mcp-bridge.test.tstest/e2e/support/mcp-bridge-tool-discovery.test.ts
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests. 2 semantic terminology decisionsTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. A maintainer can dispatch the default E2E suite against this exact revision. Recommended E2E: This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
Signed-off-by: Senthil Ravichandran <senthilr@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/e2e/support/mcp-bridge-tool-discovery.test.ts`:
- Around line 78-84: Update the afterEach teardown to capture compatibleMock and
artifactRoot, close the mock in a try block, and remove the artifact directory
in finally regardless of whether close rejects. Reset the shared compatibleMock
and artifactRoot state while preserving the existing cleanup behavior; no
additional Vitest restoration is needed.
🪄 Autofix
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: 3bed8943-fe13-4270-bb2a-1b0d7f1d0119
📒 Files selected for processing (2)
test/e2e/live/mcp-bridge-tool-discovery.tstest/e2e/support/mcp-bridge-tool-discovery.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- test/e2e/live/mcp-bridge-tool-discovery.ts
| let artifactRoot: string | undefined; | ||
|
|
||
| afterEach(async () => { | ||
| await compatibleMock?.close(); | ||
| compatibleMock = undefined; | ||
| if (artifactRoot) await fs.rm(artifactRoot, { recursive: true, force: true }); | ||
| artifactRoot = undefined; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 3 '\bvi\.(spyOn|stubEnv|stubGlobal)\b|process\.env|globalThis' \
test/e2e/support/mcp-bridge-tool-discovery.test.tsRepository: NVIDIA/NemoClaw
Length of output: 153
Make teardown unconditional.
If compatibleMock.close() rejects, artifact removal does not run. Capture compatibleMock and artifactRoot, then remove the artifact directory in a finally block. The test does not call vi.spyOn, vi.stubEnv, or vi.stubGlobal, so no additional Vitest restoration is required.
🤖 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/support/mcp-bridge-tool-discovery.test.ts` around lines 78 - 84,
Update the afterEach teardown to capture compatibleMock and artifactRoot, close
the mock in a try block, and remove the artifact directory in finally regardless
of whether close rejects. Reset the shared compatibleMock and artifactRoot state
while preserving the existing cleanup behavior; no additional Vitest restoration
is needed.
Sources: Coding guidelines, Path instructions
wscurran
added
area: security
Summary
Preserve redacted MCP tool-discovery status and request evidence before live E2E assertions run. This lets issue #8746 distinguish discovery-runtime startup, policy or credential handling, and MCP protocol failures without recording credentials or session identifiers.
Related Issue
Refs #8746
Changes
toolDiscoverystatus to the live E2E artifact set.Type of Change
Quality Gates
Documentation Writer Review
no-docs-neededDGX Station Hardware Evidence
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailablenpx vitest run --project e2e-support test/e2e/support/mcp-bridge-tool-discovery.test.tspassed 25/25;npm run build:cliandnpm run typecheck:clipassed. The OpenClaw MCP bridge passed twice on OpenShell v0.0.101 with two trusted-private tools discovered, artifact credential scans clear, and cleanup complete: run 31413260063 and run 31414578417.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: Senthil Ravichandran senthilr@nvidia.com