test(e2e): assert Hermes routing topology - #8896
Conversation
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughChangesHermes E2E tests now inspect Hermes routing topology
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant HermesE2ETest
participant captureHermesRoutingTopology
participant Sandbox
participant PythonProcProbe
HermesE2ETest->>captureHermesRoutingTopology: capture gateway topology
captureHermesRoutingTopology->>Sandbox: run trusted probe
Sandbox->>PythonProcProbe: inspect /proc metadata
PythonProcProbe-->>Sandbox: return topology JSON
Sandbox-->>captureHermesRoutingTopology: return probe result
captureHermesRoutingTopology-->>HermesE2ETest: return validated topology
HermesE2ETest->>HermesE2ETest: assert no standalone sidecars after restart
Possibly related issues
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit d770c76 in the TypeScript / code-coverage/cliThe overall coverage in commit d770c76 in the Show a code coverage summary of the most impacted files.
Updated |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
test/e2e/fixtures/hermes-routing-topology.ts (2)
180-182: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer an explicit return type over
ReturnType<typeof trustedSandboxShellScript>.The indirect type hides the public shape from readers of this fixture. Import and name the concrete type instead.
♻️ Proposed change
-export function buildHermesRoutingTopologyProbeScript( - procRoot = "/proc", -): ReturnType<typeof trustedSandboxShellScript> { +export function buildHermesRoutingTopologyProbeScript(procRoot = "/proc"): TrustedShellCommand {Add the type import:
-import type { ShellProbeRunOptions } from "./shell-probe.ts"; +import type { ShellProbeRunOptions, TrustedShellCommand } from "./shell-probe.ts";🤖 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/fixtures/hermes-routing-topology.ts` around lines 180 - 182, Update buildHermesRoutingTopologyProbeScript to use the concrete trusted sandbox shell script type as its explicit return type instead of ReturnType<typeof trustedSandboxShellScript>. Import the named type and preserve the function’s existing behavior.
266-285: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider writing the artifact before the topology assertions run downstream.
captureHermesRoutingTopologywrites the artifact only afterparseHermesRoutingTopologysucceeds. If the probe emits malformed JSON, no routing-topology artifact is persisted, and the failure diagnosis loses the captured evidence. TheexecShellartifacts still hold raw stdout, so this is not a data loss defect. Persisting a raw-capture artifact on the parse-failure path would improve triage.🤖 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/fixtures/hermes-routing-topology.ts` around lines 266 - 285, Update captureHermesRoutingTopology so malformed probe output still persists a raw-capture artifact before parseHermesRoutingTopology can fail. Keep the existing parsed topology artifact for successful parsing, and use the existing artifact-writing mechanism and artifactLabel(options.artifactName) naming context for the failure-path capture.test/e2e/support/hermes-routing-topology.test.ts (1)
136-148: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
runTopologyProbeinstead of repeating the spawn logic.This test repeats the
spawnSyncinvocation thatrunTopologyProbealready provides. Reuse of the helper keeps one spawn configuration and removes the duplicate.♻️ Proposed change
- const result = spawnSync("sh", ["-c", buildHermesRoutingTopologyProbeScript(procRoot)], { - encoding: "utf8", - }); - - expect(result.status, result.stderr).toBe(0); - expect(parseHermesRoutingTopology(result.stdout.trim())).toEqual({ + const { topology } = runTopologyProbe(procRoot); + + expect(topology).toEqual({🤖 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/hermes-routing-topology.test.ts` around lines 136 - 148, Update the test “detects both external sidecar shapes from process metadata only (`#8889`)” to call the existing runTopologyProbe helper with the generated procRoot/script instead of invoking spawnSync directly. Preserve the current probe result assertions and inputs while removing the duplicated spawn configuration.
🤖 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/hermes-e2e.test.ts`:
- Around line 747-750: Update the artifact name passed to
assertNoStandaloneRoutingSidecars in the restart path at this location so it
uniquely identifies this supervisor restart phase; also apply the matching
distinct name to the call in the other restart path near the corresponding
second occurrence. Preserve the declared semantic phase and progress-reporting
conventions while ensuring each topology capture is attributed to its relevant
restart phase.
In `@test/e2e/support/hermes-routing-topology.test.ts`:
- Around line 71-80: Bound both synchronous topology probes: in
test/e2e/support/hermes-routing-topology.test.ts lines 71-80, update
runTopologyProbe’s spawnSync options with a positive timeout shorter than the
first heartbeat and killSignal "SIGKILL"; at lines 144-146, replace the repeated
spawnSync call with runTopologyProbe so it reuses the bounded configuration.
---
Nitpick comments:
In `@test/e2e/fixtures/hermes-routing-topology.ts`:
- Around line 180-182: Update buildHermesRoutingTopologyProbeScript to use the
concrete trusted sandbox shell script type as its explicit return type instead
of ReturnType<typeof trustedSandboxShellScript>. Import the named type and
preserve the function’s existing behavior.
- Around line 266-285: Update captureHermesRoutingTopology so malformed probe
output still persists a raw-capture artifact before parseHermesRoutingTopology
can fail. Keep the existing parsed topology artifact for successful parsing, and
use the existing artifact-writing mechanism and
artifactLabel(options.artifactName) naming context for the failure-path capture.
In `@test/e2e/support/hermes-routing-topology.test.ts`:
- Around line 136-148: Update the test “detects both external sidecar shapes
from process metadata only (`#8889`)” to call the existing runTopologyProbe helper
with the generated procRoot/script instead of invoking spawnSync directly.
Preserve the current probe result assertions and inputs while removing the
duplicated spawn configuration.
🪄 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: f264a3b9-bba0-4002-a738-6566c910b2cf
📒 Files selected for processing (3)
test/e2e/fixtures/hermes-routing-topology.tstest/e2e/live/hermes-e2e.test.tstest/e2e/support/hermes-routing-topology.test.ts
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
2 terminology differences from the second opinionAdvisory only. These are normalized differences from the primary terminology receipt.
2 additional E2E selections from the second opinionAdvisory only. The primary lane did not select these E2E jobs or targets.
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 for the commit under review. Recommended E2E: None Manual-only E2E: This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
cjagwani
added
area: e2e
Security review — PASSReviewed the complete diff for the Hermes routing-topology E2E boundary against all nine repository security categories.
No security findings. This review establishes an E2E evidence baseline only; it does not claim final native routing support. |
Maintainer waiver — optional Nemotron advisor laneAccepted non-success check: The lane failed twice because its model emitted analysis before completing a required internal tool result ( I also inspected the two partial Nemotron findings rather than discarding them:
The partial blocker and warning incorrectly treated the mutually exclusive branches as if both or neither executed. No code change is warranted. Required CI, including the previously failed CLI shard 12, passed on rerun. |
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
Accepted scope is explicit in #7937 and #8889. The delta from the prior approved commit only merges current main's unrelated version-comparator change; the complete three-file PR diff remains correct. It records one supervised Hermes gateway and zero standalone Relay/Switchyard sidecars before and after either restart path. Strict parsing, exact PID binding, bounded execution, redacted artifacts, and failure-path tests limit regression and security risk. No current automated findings or competing PRs exist, and current main is fully merged.
3 participants
Summary
The live Hermes E2E previously proved gateway restart behavior without recording the process topology needed for native Relay and Switchyard qualification. It now captures normalized, credential-safe process evidence before and after restart and requires one Hermes gateway with no standalone NeMo Relay or Switchyard sidecars.
Related Issue
Part of #8889.
Changes
/proctopology probe that reads command and process identity metadata without reading process environments.nemo-relayorswitchyard-serverprocesses.Type of Change
Quality Gates
PR review advisor (Nemotron 3 Ultra)failed due its internal tool-order contract after two attempts; maintainer waiver and false-positive audit. Required CI passed.Documentation Writer Review
no-docs-neededa30fe4e24changes only internal Hermes E2E fixtures, tests, and evidence artifacts. The final commit exercises the topology probe's exact process-metadata permission-denial path. The complete PR does not change a CLI, configuration, API, policy, default, user-visible error, supported workflow, or native routing support claim.DGX 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 test/e2e/support/hermes-routing-topology.test.ts --project e2e-support(8 passed);npm run typecheck:cli; Biomenpm run docsbuilds without warnings (doc changes only)Signed-off-by: Charan Jagwani cjagwani@nvidia.com