Skip to content

test(e2e): clear Hermes rebuild dashboard forward - #3939

Merged
ericksoa merged 1 commit into
mainfrom
fix/rebuild-hermes-dashboard-forward-0520
May 20, 2026
Merged

test(e2e): clear Hermes rebuild dashboard forward#3939
ericksoa merged 1 commit into
mainfrom
fix/rebuild-hermes-dashboard-forward-0520

Conversation

@ericksoa

@ericksoa ericksoa commented May 20, 2026

Copy link
Copy Markdown
Contributor

Summary

  • stop the prior Hermes dashboard/API forward after the Phase 1 sandbox delete in the Hermes rebuild E2E
  • prevents the rebuild recreate path from tripping over a stale host-bound 8642 forward during image build

Verification

  • bash -n test/e2e/test-rebuild-hermes.sh
  • git diff --check

Follow-up to main nightly https://github.com/NVIDIA/NemoClaw/actions/runs/26194332461 where rebuild-hermes-e2e failed on the stale dashboard forward.

Summary by CodeRabbit

  • Tests
    • Updated the Hermes rebuild end-to-end test to improve the sandbox recreation sequence.

Review Change Stack

@coderabbitai

coderabbitai Bot commented May 20, 2026

Copy link
Copy Markdown
Contributor

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 0cbe6637-1859-4799-8d85-6ff558d5fd25

📥 Commits

Reviewing files that changed from the base of the PR and between 419a3c3 and 13a55e2.

📒 Files selected for processing (1)
  • test/e2e/test-rebuild-hermes.sh

📝 Walkthrough

Walkthrough

The Hermes rebuild E2E test script now stops the OpenShell port 8642 forward before deleting the Phase 1-created sandbox and creating the older Hermes fixture sandbox. This prevents a short-lived host-bound dashboard/API forward window during the rebuild flow.

Changes

Hermes Rebuild E2E Test

Layer / File(s) Summary
OpenShell forward cleanup before sandbox rebuild
test/e2e/test-rebuild-hermes.sh
Script adds best-effort stop of existing OpenShell forward on port 8642 before proceeding to delete Phase 1-created sandbox and create the Phase 2 Hermes fixture sandbox.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~5 minutes

Suggested reviewers

  • cjagwani
  • jyaunches

Poem

🐰 A forward port was bound too tight,
So stop it first—then do what's right!
Before the sandbox starts to dance,
Clean up the dash' for rebuild's chance. ✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and specifically describes the main change: clearing the Hermes rebuild dashboard forward to fix a stale port forward issue during the E2E test rebuild flow.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/rebuild-hermes-dashboard-forward-0520

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions

Copy link
Copy Markdown
Contributor

Selective E2E Results — ❌ Some jobs failed

Run: 26194792170
Target ref: 13a55e25cfb574739d6d843c3507e8c704fcf99b
Workflow ref: main
Requested jobs: rebuild-hermes-e2e,rebuild-hermes-stale-base-e2e
Summary: 0 passed, 2 failed, 0 skipped

Job Result
rebuild-hermes-e2e ❌ failure
rebuild-hermes-stale-base-e2e ❌ failure

Failed jobs: rebuild-hermes-e2e, rebuild-hermes-stale-base-e2e. Check run artifacts for logs.

@github-actions

Copy link
Copy Markdown
Contributor

E2E Advisor Recommendation

Required E2E: None
Optional E2E: rebuild-hermes-e2e, rebuild-hermes-stale-base-e2e

Dispatch hint: rebuild-hermes-e2e,rebuild-hermes-stale-base-e2e

Workflow run

Full advisor summary

E2E Recommendation Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required E2E

  • None. No merge-blocking E2E is required because this PR changes only an existing E2E test harness file and does not modify installer, onboarding, sandbox lifecycle runtime code, credentials, policy, inference routing, deployment, or assistant user-flow implementation.

Optional E2E

  • rebuild-hermes-e2e (medium): Validates that the modified test harness still passes for the standard Hermes rebuild upgrade path that uses test/e2e/test-rebuild-hermes.sh.
  • rebuild-hermes-stale-base-e2e (medium): Validates the stale cached Hermes base-image rebuild mode of the same script, which is especially relevant because the new cleanup is intended to avoid host-bound dashboard port interference before creating the old fixture sandbox.

New E2E recommendations

  • None.

Dispatch hint

  • Workflow: .github/workflows/nightly-e2e.yaml
  • jobs input: rebuild-hermes-e2e,rebuild-hermes-stale-base-e2e

@github-actions

Copy link
Copy Markdown
Contributor

PR Review Advisor

Recommendation: blocked
Confidence: medium
Analyzed HEAD: 13a55e25c9c319b3d04bb023d3b6bc83acbbcae7
Findings: 2 blocker(s), 2 warning(s), 0 suggestion(s)

This is an automated advisory review. A human maintainer must make the final merge decision.

Limitations: Review is based on trusted deterministic context and the provided diff; no PR scripts, tests, package-manager commands, or network operations were executed.; CI, CodeRabbit, CodeQL, ShellCheck, E2E recommendation, and platform E2E jobs were still pending or in progress in the provided context.; No linked issues were provided; acceptance coverage maps literal PR body clauses and the included follow-up statement instead.; Review thread state is limited: GraphQL reviewThreads.nodes is empty, but CodeRabbit had only posted an in-progress comment and its status context was pending.

Workflow run

Full advisor summary

PR Review Advisor

Base: origin/main
Head: HEAD
Analyzed SHA: 13a55e25c9c319b3d04bb023d3b6bc83acbbcae7
Recommendation: blocked
Confidence: medium

The patch is a narrow E2E cleanup change to stop a stale Hermes dashboard/API forward, but merge is currently blocked by pending CI/E2E advisor state and mergeStateStatus=BLOCKED.

Gate status

  • CI: pending — Head SHA 13a55e2 has multiple pending/in-progress/queued contexts, including E2E recommendation, wsl-e2e, macos-e2e, PR review advisor, CodeQL, ShellCheck SARIF, unit-vitest-linux, checks, build-sandbox-images, build-sandbox-images-arm64, and CodeRabbit.
  • Mergeability: fail — GraphQL mergeStateStatus=BLOCKED for PR test(e2e): clear Hermes rebuild dashboard forward #3939 at head SHA 13a55e2.
  • Review threads: unknown — GraphQL reviewThreads.nodes is empty, but CodeRabbit posted an in-progress review comment and the CodeRabbit status context is PENDING.
  • Risky code tested: pass — Trusted path heuristics reported no risky code areas; the diff is limited to test/e2e/test-rebuild-hermes.sh and adds an E2E cleanup command.

🔴 Blockers

  • Required CI and analysis checks are still pending for the head SHA: The latest head SHA has pending/in-progress/queued contexts, including E2E recommendation, platform E2E jobs, CodeQL, ShellCheck, unit tests, sandbox image builds, and CodeRabbit. These gates must complete before the PR can be assessed for merge readiness.
    • Recommendation: Wait for all required checks for 13a55e2 to complete successfully and re-review any failures.
    • Evidence: statusCheckRollup shows multiple contexts with status IN_PROGRESS, QUEUED, or PENDING; deterministic gateStatus.ci reports 12 pending status contexts.
  • PR merge state is blocked: GitHub reports the PR as blocked, so it is not currently mergeable even though the file-level change is small.
    • Recommendation: Resolve the branch protection, required review, and pending-check requirements that are causing mergeStateStatus=BLOCKED.
    • Evidence: GraphQL pullRequest.mergeStateStatus=BLOCKED and deterministic gateStatus.mergeability.status=fail.

🟡 Warnings

  • Potential overlap with active Hermes upgrade work (test/e2e/test-rebuild-hermes.sh): The changed E2E file overlaps with another open PR touching the same file, so there is a codebase drift or integration risk even though this patch applies to an existing file.
    • Recommendation: Before merging, compare this cleanup with PR chore: upgrade Hermes to v2026.5.16 #3831 and ensure the Hermes upgrade work does not already change or supersede the rebuild flow around dashboard forwarding.
    • Evidence: Trusted overlap data reports open PR chore: upgrade Hermes to v2026.5.16 #3831, "chore: upgrade Hermes to v2026.5.16", with sameFiles=["test/e2e/test-rebuild-hermes.sh"]. Recent history also shows active Hermes rebuild/messaging changes on this file.
  • Behavioral fix is in an E2E path but E2E proof is not yet available for this head SHA (test/e2e/test-rebuild-hermes.sh:114): The new command changes the Hermes rebuild E2E lifecycle by synchronously stopping host forward 8642 after deleting the Phase 1 sandbox. That is the right area for the reported stale-forward failure, but the relevant E2E recommendation and platform E2E checks are still running.
    • Recommendation: Use the completed E2E recommendation and any required rebuild-Hermes/platform E2E jobs for this exact head SHA as the acceptance evidence before considering the PR ready.
    • Evidence: Diff adds openshell forward stop 8642 >/dev/null 2>&1 || true; statusCheckRollup shows E2E recommendation, wsl-e2e, and macos-e2e are IN_PROGRESS.

🔵 Suggestions

  • None.

Acceptance coverage

  • met — stop the prior Hermes dashboard/API forward after the Phase 1 sandbox delete in the Hermes rebuild E2E: The diff adds openshell forward stop 8642 >/dev/null 2>&1 || true immediately after openshell sandbox delete "${SANDBOX_NAME}" 2>/dev/null || true in test/e2e/test-rebuild-hermes.sh.
  • partial — prevents the rebuild recreate path from tripping over a stale host-bound 8642 forward during image build: The added cleanup targets port 8642, matching the stated stale host-bound forward. However, CI/E2E jobs for the head SHA are still pending, so runtime proof that the rebuild recreate path no longer trips is not yet available.
  • unknown — bash -n test/e2e/test-rebuild-hermes.sh: The PR body claims this verification, but trusted CI evidence provided here does not include a completed bash syntax check result for this exact command.
  • unknown — git diff --check: The PR body claims this verification, but trusted CI evidence provided here does not include a completed git diff --check result.
  • partial — Follow-up to main nightly https://github.com/NVIDIA/NemoClaw/actions/runs/26194332461 where rebuild-hermes-e2e failed on the stale dashboard forward.: The diff directly targets stale dashboard forward cleanup by stopping forward 8642 after raw sandbox deletion. Full follow-up validation is pending because E2E recommendation and platform E2E checks are still in progress.

Security review

  • pass — 1. Secrets and Credentials: No hardcoded secrets, tokens, passwords, key files, or credential material are introduced. The added line stops an OpenShell forward and redirects output to /dev/null.
  • pass — 2. Input Validation and Data Sanitization: No new user-controlled parsing, deserialization, path handling, URL handling, shell interpolation, or network input processing is introduced. The port argument is a fixed literal 8642.
  • pass — 3. Authentication and Authorization: No authentication or authorization logic is modified. The change is confined to an E2E test cleanup step.
  • pass — 4. Dependencies and Third-Party Libraries: No dependencies, registries, package manifests, or third-party version pins are changed.
  • pass — 5. Error Handling and Logging: The new cleanup is best-effort with || true, consistent with surrounding teardown-style commands. It does not log secrets or expose additional internal state.
  • pass — 6. Cryptography and Data Protection: Not applicable — no cryptographic operations or data protection mechanisms are changed.
  • pass — 7. Configuration and Security Headers: No production configuration, HTTP headers, CORS/CSP policy, Dockerfile, container privilege model, or port exposure policy is changed. The added operation is limited to stopping a fixed E2E forward.
  • warning — 8. Security Testing: The change improves an E2E lifecycle cleanup path involving sandbox forwarding, but required CI/E2E evidence for the head SHA is still pending. Wait for E2E recommendation and related jobs before relying on this as tested.
  • pass — 9. Holistic Security Posture: The patch does not weaken sandbox isolation, SSRF defenses, credential handling, blueprint integrity, installer trust, or workflow trusted-code boundaries. Stopping a stale forward after sandbox deletion is security-neutral to positive for test isolation.

Test / E2E status

  • Test depth: unit_sufficient — Trusted analysis classifies this as limited to tests, documentation, or metadata that cannot affect runtime behavior directly. The change is in an E2E shell script and does not modify production code, though the relevant E2E checks should still complete because the behavior being fixed is an E2E lifecycle failure.
  • E2E Advisor: missing
  • Required E2E jobs: E2E recommendation
  • Missing for analyzed SHA: E2E recommendation

✅ What looks good

  • The patch is narrow: one changed file with five inserted lines and no production runtime code changes.
  • The added cleanup is placed at the correct lifecycle point, immediately after raw OpenShell sandbox deletion and before creating the old fixture sandbox.
  • The command uses a fixed port literal and best-effort failure handling, avoiding new input or command-injection surfaces.
  • No secrets, dependencies, workflow files, Dockerfiles, installers, or credential-handling code are changed.

Review completeness

  • Review is based on trusted deterministic context and the provided diff; no PR scripts, tests, package-manager commands, or network operations were executed.
  • CI, CodeRabbit, CodeQL, ShellCheck, E2E recommendation, and platform E2E jobs were still pending or in progress in the provided context.
  • No linked issues were provided; acceptance coverage maps literal PR body clauses and the included follow-up statement instead.
  • Review thread state is limited: GraphQL reviewThreads.nodes is empty, but CodeRabbit had only posted an in-progress comment and its status context was pending.
  • Human maintainer review required: yes

@github-actions

Copy link
Copy Markdown
Contributor

Selective E2E Results — ✅ All requested jobs passed

Run: 26194833233
Target ref: 13a55e25c9c319b3d04bb023d3b6bc83acbbcae7
Workflow ref: main
Requested jobs: rebuild-hermes-e2e,rebuild-hermes-stale-base-e2e
Summary: 2 passed, 0 failed, 0 skipped

Job Result
rebuild-hermes-e2e ✅ success
rebuild-hermes-stale-base-e2e ✅ success

@ericksoa
ericksoa merged commit 7207fe1 into main May 20, 2026
30 checks passed
@ericksoa
ericksoa deleted the fix/rebuild-hermes-dashboard-forward-0520 branch May 20, 2026 23:09
@wscurran wscurran added the chore Build, CI, dependency, or tooling maintenance label Jun 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

chore Build, CI, dependency, or tooling maintenance

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants