test(e2e): surface Hermes MCP rebuild failures - #8659
Closed
apurvvkumaria wants to merge 1 commit into
Closed
Conversation
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Contributor
📝 WalkthroughWalkthroughThe PR adds post-redaction output limits to ChangesHermes MCP diagnostics
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant mcpBridgeTest as mcp-bridge.test
participant shellProbe as ShellProbe
participant hermesEndpoint as Hermes MCP HTTP endpoint
participant requestObservations as McpRequestObservation
mcpBridgeTest->>shellProbe: Run the shared Hermes chat probe
shellProbe->>hermesEndpoint: Send one authenticated /mcp request
hermesEndpoint-->>shellProbe: Return bounded status and result markers
shellProbe-->>mcpBridgeTest: Return redacted diagnostic output
mcpBridgeTest->>requestObservations: Assert one authenticated tools/call request
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
Contributor
Contributor
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. 3 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/e2e/support/mcp-bridge-hermes-http.test.ts (1)
383-411: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the transport-failure and missing-status branches.
This test covers the 2xx token rules and the tools/call count. Three failure branches of the new helpers stay uncovered:
assertHermesMcpHttpResponsetransport branch, whenexitCode !== 0orsignalis set. Thetransport=text and the joined stdout/stderr preview are never asserted.assertHermesMcpHttpResponsestatus branch, whenstderrcarries zero or two status markers.parseHttpStatusreturnsnullfor both cases, and the "did not report one bounded HTTP status" message is never asserted.assertAuthenticatedMcpToolCallOutcomepath and credential branches at lines 131 and 134 oftest/e2e/live/mcp-bridge-hermes-http.ts.These branches produce the terminal evidence that the live Hermes target depends on. A regression in them would surface only during a live run.
🧪 Proposed additional cases
expect(() => assertAuthenticatedMcpToolCallOutcome({ requests: [request, request], callsBefore: 0, expectedSecret: "rotated-fixture-secret", }), ).toThrowError(/exactly one tools\/call/u); + expect(() => + assertAuthenticatedMcpToolCallOutcome({ + requests: [{ ...request, path: "/other" }], + callsBefore: 0, + expectedSecret: "rotated-fixture-secret", + }), + ).toThrowError(/managed \/mcp endpoint/u); + expect(() => + assertAuthenticatedMcpToolCallOutcome({ + requests: [request], + callsBefore: 0, + expectedSecret: "unexpected-secret", + }), + ).toThrowError(/expected resolved bearer credential/u); + }); + + it("reports bounded redacted evidence for transport and status failures", () => { + const secret = "fixture-transport-secret"; + expect(() => + assertHermesMcpHttpResponse( + { exitCode: 7, signal: null, stdout: secret, stderr: "" }, + [secret], + ), + ).toThrowError(/transport failed \(exit=7\)/u); + expect(() => + assertHermesMcpHttpResponse( + { exitCode: null, signal: "SIGKILL", stdout: "", stderr: "" }, + [], + ), + ).toThrowError(/transport failed \(signal=SIGKILL\)/u); + expect(() => + assertHermesMcpHttpResponse({ exitCode: 0, signal: null, stdout: "", stderr: "" }, []), + ).toThrowError(/one bounded HTTP status/u); + expect(() => + assertHermesMcpHttpResponse( + { + exitCode: 0, + signal: null, + stdout: "", + stderr: `${HERMES_MCP_HTTP_STATUS_MARKER}200\n${HERMES_MCP_HTTP_STATUS_MARKER}500\n`, + }, + [], + ), + ).toThrowError(/one bounded HTTP status/u); });Also assert that the transport message excludes the raw secret:
try { assertHermesMcpHttpResponse( { exitCode: 7, signal: null, stdout: "leak-me-secret", stderr: "" }, ["leak-me-secret"], ); } catch (error) { const message = error instanceof Error ? error.message : String(error); expect(message).not.toContain("leak-me-secret"); expect(message).toContain("[REDACTED]"); }🤖 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-hermes-http.test.ts` around lines 383 - 411, Extend the test around assertHermesMcpHttpResponse to cover nonzero exitCode or signal transport failures, asserting the transport message includes joined stdout/stderr evidence while redacting supplied secrets. Add cases for zero and multiple HTTP status markers in stderr, asserting the bounded-status error. Extend assertAuthenticatedMcpToolCallOutcome coverage for invalid request paths and credentials, targeting its path and credential validation branches.
🤖 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/e2e/support/mcp-bridge-hermes-http.test.ts`:
- Around line 383-411: Extend the test around assertHermesMcpHttpResponse to
cover nonzero exitCode or signal transport failures, asserting the transport
message includes joined stdout/stderr evidence while redacting supplied secrets.
Add cases for zero and multiple HTTP status markers in stderr, asserting the
bounded-status error. Extend assertAuthenticatedMcpToolCallOutcome coverage for
invalid request paths and credentials, targeting its path and credential
validation branches.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 18379dac-a23f-4b42-ad2b-8b08224c6c35
📒 Files selected for processing (6)
test/e2e/README.mdtest/e2e/fixtures/shell-probe.tstest/e2e/live/mcp-bridge-hermes-http.tstest/e2e/live/mcp-bridge.test.tstest/e2e/support/e2e-redaction-entry.test.tstest/e2e/support/mcp-bridge-hermes-http.test.ts
This was referenced Aug 10, 2026
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
1 participant
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
Hermes MCP post-rebuild checks now preserve bounded, redacted HTTP failure evidence while keeping successful responses content-free. This makes the first authenticated MCP failure diagnosable without retrying the request or exposing the sandbox API key.
Related Issue
Contributes to #8590.
Changes
test/e2e/README.md.Type of Change
Quality Gates
Documentation Writer Review
docs-updatedtest/e2e/README.mdDGX 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-hermes-http.test.tspassed 4/4 and the secret scan passed.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result: Not applicable; this is a focused live-E2E diagnostic and support-test change.npm run docsbuilds without warnings (doc changes only)Signed-off-by: Apurv Kumaria akumaria@nvidia.com
Summary by CodeRabbit
Bug Fixes
Tests