Skip to content

fix(hermes): guard ui-tui and web npm ci steps for older Hermes versions#4476

Closed
hunglp6d wants to merge 1 commit into
mainfrom
fix/nightly-e2e-hermes-dockerfile-npm-ci-compat-d3e0f3b
Closed

fix(hermes): guard ui-tui and web npm ci steps for older Hermes versions#4476
hunglp6d wants to merge 1 commit into
mainfrom
fix/nightly-e2e-hermes-dockerfile-npm-ci-compat-d3e0f3b

Conversation

@hunglp6d
Copy link
Copy Markdown
Contributor

Summary

Guard the ui-tui and web npm ci / build steps in agents/hermes/Dockerfile.base with [ -f <dir>/package-lock.json ] existence checks so the Dockerfile stays compatible with older Hermes releases (v2026.4.13) that lack these subdirectories.

Commit 40ea247 (#4442 "feat(hermes): expose optional web dashboard") added unconditional npm ci --prefix ui-tui and npm ci --prefix web steps. The rebuild-hermes E2E tests build Dockerfile.base with HERMES_VERSION=v2026.4.13, which predates the ui-tui/ and web/ directories — causing npm ci to fail with EUSAGE ("no existing package-lock.json").

Related Issue

Linked after issue creation below.

Changes

  • Wrap npm ci --prefix ui-tui + npm run build --prefix ui-tui in if [ -f ui-tui/package-lock.json ]
  • Wrap npm ci --prefix web + npm run build --prefix web in if [ -f web/package-lock.json ]
  • No functional change for current Hermes (v2026.5.16) which has both lockfiles
  • Restores backward compatibility for the rebuild-hermes E2E test matrix

Validation

Note: The custom-e2e validation branch could not be created because the GitHub token lacks the workflows permission needed to push files under .github/workflows/. The fix is a mechanical conditional guard on a Docker RUN step and has been verified by code inspection against the build logs.

  • Original failing run: 26610274905 on d3e0f3ba27969847591c00af08a6c79681ba24e8
  • Targeted jobs: rebuild-hermes-e2e / run (#78414410806), rebuild-hermes-stale-base-e2e / run (#78414410803)

Type of Change

  • 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)

Verification

  • npx prek run --all-files passes
  • npm test passes
  • Tests added or updated for new or changed behavior
  • No secrets, API keys, or credentials committed
  • Docs updated for user-facing behavior changes

AI Disclosure

  • AI-assisted — tool: Claude Code (nemoclaw-diagnosis skill)

Signed-off-by: Hung Le hple@nvidia.com

The rebuild-hermes E2E tests build agents/hermes/Dockerfile.base with
HERMES_VERSION=v2026.4.13, which predates the ui-tui/ and web/
subdirectories added in v2026.5.16.  The unconditional `npm ci --prefix
ui-tui` fails with EUSAGE because the old tarball has no
ui-tui/package-lock.json.

Wrap the ui-tui and web npm-ci + build steps in `[ -f <dir>/package-lock.json ]`
guards so the Dockerfile stays compatible with older Hermes releases used by
the stale-base rebuild test matrix.

Fixes rebuild-hermes-e2e and rebuild-hermes-stale-base-e2e nightly failures
on run 26610274905.

Signed-off-by: Hung Le <hple@nvidia.com>
@copy-pr-bot
Copy link
Copy Markdown

copy-pr-bot Bot commented May 29, 2026

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 29, 2026

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 6beb52c8-4692-4312-aaa6-cd1d3f2088f9

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/nightly-e2e-hermes-dockerfile-npm-ci-compat-d3e0f3b

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

@github-actions
Copy link
Copy Markdown
Contributor

PR Review Advisor

Findings: 0 needs attention, 2 worth checking, 0 nice ideas
Top item: Validate and tighten optional frontend skip behavior

Review findings

🛠️ Needs attention

  • None.

🔎 Worth checking

  • Source-of-truth review needed: Hermes Dockerfile optional frontend build compatibility: The advisor marked localized patch analysis as needs_followup.
    • Recommendation: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
    • Evidence: The changed Dockerfile conditionally runs optional frontend `npm ci`/build steps only when each subproject lockfile exists.
  • Compatibility guard lacks targeted runtime validation and may mask packaging drift (agents/hermes/Dockerfile.base:179): The new guard treats a missing `ui-tui/package-lock.json` or `web/package-lock.json` as an optional skip. That matches the stated older-Hermes compatibility case, but it also means a future/current Hermes tarball that contains a frontend directory but accidentally omits its lockfile would build without those assets instead of failing. The source-of-truth workaround is plausible because historical upstream tarballs cannot be changed here, but the PR does not add or identify in-repo targeted validation for the old-tag skip path and the current-tag build path.
    • Recommendation: Add or point to targeted runtime/integration validation that builds `agents/hermes/Dockerfile.base` with the old Hermes fixture (`v2026.4.13`) and the current default, and consider tightening the shell logic so only an absent optional directory is skipped while a present directory without `package-lock.json` fails loudly.
    • Evidence: The diff changes unconditional `npm ci --prefix ui-tui` and `npm ci --prefix web` into `[ -f .../package-lock.json ]` guarded blocks. Existing `test/e2e/test-rebuild-hermes.sh` builds `Dockerfile.base` with `OLD_HERMES_VERSION="v2026.4.13"`, but this PR does not add or modify tests for the new conditional behavior.

🌱 Nice ideas

  • None.

Workflow run details

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

@github-actions
Copy link
Copy Markdown
Contributor

E2E Advisor Recommendation

Required E2E: hermes-root-entrypoint-smoke-e2e
Optional E2E: hermes-dashboard-e2e, rebuild-hermes-e2e

Dispatch hint: hermes-root-entrypoint-smoke-e2e

Workflow run

Full advisor summary

E2E Recommendation Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required E2E

  • hermes-root-entrypoint-smoke-e2e (medium-high): Directly builds agents/hermes/Dockerfile.base and the Hermes production image, then starts the root entrypoint and verifies Hermes health, privilege separation, and runtime layout. This is the most targeted existing E2E for a Hermes base-image build change.

Optional E2E

  • hermes-dashboard-e2e (high): Useful follow-up confidence for the web/dashboard-adjacent npm build changes: exercises Hermes install/onboard with the optional dashboard enabled and verifies dashboard/API forwards and host reachability. Optional because it is a longer live-inference flow and the required root-entrypoint smoke directly validates the changed base-image build.
  • rebuild-hermes-e2e (high): Validates Hermes rebuild behavior with the current Hermes base image, which is adjacent to base-image changes. Optional because this PR only changes base dependency build guards, not rebuild orchestration.

New E2E recommendations

  • hermes-base-image-pr-selection (high): The standard Hermes production-image build path can use a published GHCR Hermes base image unless the resolver is forced to build locally. Add coverage that ensures PR changes to agents/hermes/Dockerfile.base force a local Hermes base build instead of silently testing against stale latest.
    • Suggested test: Hermes base-image PR diff detection / local-base selection E2E

Dispatch hint

  • Workflow: nightly-e2e.yaml
  • jobs input: hermes-root-entrypoint-smoke-e2e

@github-actions
Copy link
Copy Markdown
Contributor

E2E Scenario Advisor Recommendation

Required scenario E2E: None
Optional scenario E2E: None

Workflow run

Full scenario advisor summary

E2E Scenario Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required scenario E2E

  • None. No scenario workflow, scenario metadata, scenario runtime, or validation-suite files changed.

Optional scenario E2E

  • None.

Relevant changed files

  • None.

@wscurran wscurran added Docker Support for Docker containerization fix Integration: Hermes labels May 29, 2026
@wscurran
Copy link
Copy Markdown
Contributor


Related open PRs:

@jyaunches
Copy link
Copy Markdown
Contributor

Closing as superseded — an equivalent fix for the same root cause landed on main ~75 minutes after this PR was opened:

  • Commit ce6099b2 "fix(hermes): tolerate missing optional ui packages" (aerickson, 2026-05-29 02:33 UTC)

Same approach (guard the npm ci --prefix ui-tui / web steps in agents/hermes/Dockerfile.base with [ -f <dir>/package-lock.json ]), but the merged version is DRY (single for ui_dir in ui-tui web loop), logs an explicit "Skipping optional Hermes UI package…" message, and ships with a unit test (test/hermes-share-mount-deps.test.ts).

Thanks @hunglp6d for the quick, correct diagnosis on issue #4477 — the analysis here directly matched the merged fix. Closing this PR; #4477 will be verified by dispatching rebuild-hermes-e2e and rebuild-hermes-stale-base-e2e against main.

@jyaunches
Copy link
Copy Markdown
Contributor

Superseded by ce6099b on main. See prior comment.

@jyaunches jyaunches closed this May 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Docker Support for Docker containerization fix Integration: Hermes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants