perf(e2e): skip throwaway Hermes onboarding - #7244
Conversation
|
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. |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit 8263aae in the TypeScript / code-coverage/cliThe overall coverage in commit 8263aae in the Show a code coverage summary of the most impacted files.
Updated |
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
PR Review Advisor — InformationalAdvisor assessment: Informational / low confidence Model lanes
Nemotron output stays in workflow artifacts and does not change the assessment above. E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: 1 warning · 0 suggestionsWarningsWarnings do not block.
|
|
/nvskills-ci |
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
|
@apurvvkumaria overlap check before extending this stack: #7213 now covers the same Hermes bootstrap surface plus the immutable fixture, provenance, exact current-base reuse, lifecycle assertions, and measured 38–42% median improvement. A repository comparator pass scored #7213 at 14.5/16 versus this narrower draft at 13/16; #7213 covers nearly all #7144 criteria, while this draft uniquely removes the phase-one current sandbox and starts the pinned gateway directly. Could you confirm whether #7213 should supersede #7244, or whether the direct-gateway bootstrap remains desirable as a follow-on after #7213? I am keeping this PR draft and will not combine the conflicting designs without that direction. |
wscurran
added
area: ci
dfernandez365-rgb
left a comment
dfernandez365-rgb
left a comment
There was a problem hiding this comment.
Independent exact-head sensitive-path review of 8263aaeeb19cc5c353707b5e2c00544bf4b3d83d: FAIL. The performance direction is reasonable, but the replacement bootstrap removes state that authoritative rebuild requires and seeds a port the production preflight explicitly rejects.
-
HIGH — Configure the required OpenShell inference route before rebuild.
test/e2e/live/rebuild-hermes.test.ts:545-561starts and probes only the gateway. The provider created later at lines 622-642 is the Discord credential provider, not an inference route. Seeding registry/session JSON does not configure gateway state.preflightAuthoritativeRebuildTarget()requiresinferenceRouteReady(target.provider, target.model)unless a prepared-backup deferral is explicit (src/lib/onboard/authoritative-rebuild-target.ts:196-210), and the helper proves that state throughopenshell inference get -g <gateway>(src/lib/onboard/inference-route.ts:18-39). This exact test should therefore fail before destructive rebuild work. Configure the compatible provider/model route through the supported OpenShell interface and assert the exact live route before invoking rebuild. -
HIGH — Do not persist Hermes API port 8642 as
dashboardPort.HERMES_API_FORWARD_PORTis 8642, assigned tophase1DashboardPortattest/e2e/live/rebuild-hermes.test.ts:562, then written into the curated registry at lines 375 and 758-760. Production defines 8642 as the reserved Hermes OpenAI-compatible API port, never a dashboard port (src/lib/core/ports.ts:62-63); the Hermes dashboard is separately exposed on 18789 (agents/hermes/manifest.yaml:35-48). Authoritative preflight callsassertDashboardPortNotReserved()and deterministically rejects 8642 (src/lib/onboard/authoritative-rebuild-target.ts:212-214,src/lib/onboard/preflight-ports.ts:45-54). Preserve separate API and dashboard/control-UI ports and add a regression that the curated target passes authoritative preflight. -
HIGH correctness / MEDIUM trust — Bind execution to the OpenShell binary that this workflow installs.
.github/workflows/e2e.yaml:2284-2300installs in one step, then the test invokes bareopenshell. In noninteractive mode the installer can write$HOME/.local/bin/openshelland explicitly warns that PATH must be updated (scripts/install-openshell.sh:791-822); it may also reuse/replace another compatible location. The test and fixture clients default toopenshell, so the job can fail to find the installed binary or execute a stale system binary. Follow the nearby canonical workflow pattern: resolve an absolute selected binary, prepend$HOME/.local/bin, exportOPENSHELL_BIN, and record/verify the selected version and provenance before the test. -
MEDIUM — The new credential-scrubbing boundary is lexical and untested.
tools/e2e/workflow-boundary.mts:1603-1614accepts raw-u NAMEsubstrings, so comments/dead shell text can satisfy it, and no negative mutation test removes any required scrubber from either job. The existing advisor warning is correct. It is also incomplete:scripts/install-openshell.sh:742prefersGH_TOKENoverGITHUB_TOKEN, but the new step unsets onlyGITHUB_TOKEN. Validate the actual command/order, unset both tokens, and add negative mutations for every scrubbed variable in both jobs.
The exact-head credentialed rebuild jobs did not run: the PR E2E gate stopped at maintainer authorization/observer timeout. Static green checks therefore do not rebut the deterministic route and reserved-port failures above. Please keep this draft until both live rebuild lanes execute against the repaired exact head and upload artifacts.
<!-- markdownlint-disable MD041 --> ## Summary <!-- 1-3 plain sentences: what changes and why. Describe before-and-after behavior when it applies. Follow the NemoClaw Writing Guide: https://github.com/NVIDIA/NemoClaw/blob/main/WRITING.md. Do not add unrelated prose cleanup. --> Rebuilds #7245 on current `main` and centralizes larger-runner selection for resource-heavy E2E lanes. Trusted final-main executions can use one administrator-configured label, while unset, PR-checkout, and untrusted contexts continue to use `ubuntu-latest`. Exact-head PR gates now add a bounded swap fallback for the hosted Hermes image-building lanes that intentionally remain on standard runners. ## Related Issue <!-- Fixes #NNN or Closes #NNN. Remove this section if none. --> Part of #7145. Supersedes #7245, whose protected head branch is stacked on obsolete #7244 history. ## Changes <!-- List concrete changes. If this adds an abstraction, configuration, fallback, migration, or compatibility path, name its current requirement and consumer, explain why a direct change is insufficient, and identify the test that protects it. --> - Build one validated runner map in `generate-matrix` before target code is checked out, consuming `E2E_LARGER_RUNNER_LABEL` only for `NVIDIA/NemoClaw` on `refs/heads/main` with no alternate checkout SHA. - Route common egress, Hermes rebuilds, heavy MCP shards, and the Hermes lanes repeatedly interrupted in run 29897237525 through that map. - Keep the OpenClaw matrix counterparts, MCP OpenClaw/dev lanes, and unrelated jobs on standard runners. - For exact-head PR gates only, reuse at least 32 GiB of active swap or provision one fixed 32 GiB file before the hosted Hermes image-building lanes. The setup uses absolute non-interactive executables in a clean environment and fails closed before Vitest. - Exclude scheduled/manual `main`, larger-runner, workflow-managed rebuild, dedicated-runner, MCP dev, and non-Hermes executions from the swap fallback. - Document the runner sizing, concurrency, comparison, fallback, and rollback requirements. - Protect the centralized rollout, exact-head fallback, and trust boundary with executable tests. A workflow-only swap step cannot help an alternate checkout until that workflow revision reaches trusted `main`. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [x] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates <!-- Check one tests line and one docs line. Check other lines when applicable. Add every requested justification or approval reference. --> - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [x] Docs updated for user-facing behavior changes - [ ] Docs not applicable — justification: - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: Independent focused review passed with no blockers after exact-head-only activation, absolute privileged executables, a clean fixed environment, and active-swap-safe cleanup were enforced at `af1cb1c77`. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Documentation Writer Review - [x] Documentation writer subagent reviewed the completed implementation - Result: `docs-updated` - Evidence: `test/e2e/README.md` documents the larger-runner trust boundary and the bounded exact-head swap fallback, selected Hermes lanes, fail-closed behavior, exclusions, sizing, measurement, and rollback requirements. - Agent: Codex Desktop - PR: #7391 <!-- docs-review-head-sha: 0d92d12 --> <!-- docs-review-agents-blob-sha: 560ff38 --> ## DGX Station Hardware Evidence <!-- Required only when scripts/prepare-dgx-station-host.sh changes. Maintainers must review the linked evidence before approving or merging. This is human-reviewed evidence, not authenticated hardware provenance. Exceptional bypasses use existing repository governance and must be documented on the PR. --> - [ ] Tested on DGX Station - Tested commit: - Station profile/scenario: - Result: - Supporting evidence: ## Verification <!-- Check each applicable item only when supported by the requested evidence. Run targeted tests once per relevant change set and rerun after later edits or hook autofixes that can affect the tested behavior. Do not rerun hook-covered checks. --> - [x] PR description includes a `Signed-off-by:` line and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run check:diff` passed when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — 59 focused E2E-support tests passed after the current `main` sync. - [x] Applicable broad gate passed — `npm run checks`, `npm run typecheck:cli`, and `npm run docs` passed; Fern reported 0 errors and 2 non-fatal warnings. - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `npm run docs` builds without warnings (doc changes only) - [ ] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) --- <!-- DCO sign-off is required in this PR description, and every commit must appear as Verified in GitHub. Run: git config user.name && git config user.email --> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * E2E workflows now generate a trusted runner-routing map and use it to route selected “heavy” lanes to a configurable larger Ubuntu runner, with validation and safe fallback to `ubuntu-latest`. * Live Vitest E2E runs can now provision a bounded Hermes swap file beforehand when required, and fail closed if provisioning fails. * **Documentation** * Added guidance for enabling, validating boundaries, and rolling back larger-runner routing. * **Tests** * Added e2e boundary coverage for runner routing and expanded Hermes swap provisioning/selection coverage. * **CI / Chores** * Updated workflow boundary checks and routing assertions to match the new trusted routing behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Charan Jagwani <cjagwani@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com> Signed-off-by: Carlos Villela <cvillela@nvidia.com> Co-authored-by: Charan Jagwani <cjagwani@nvidia.com> Co-authored-by: Carlos Villela <cvillela@nvidia.com>
|
I am working on the #7144 current-main refresh and noticed that this PR is the overlapping implementation. I have not pushed the local branch or opened a competing PR. The current-main version removes the disposable current Hermes onboard/sandbox, resolves only the published Dockerfile-pinned base, bootstraps the product gateway/inference route directly, preserves the existing rebuild/state assertions, and adds focused provenance, route, and dashboard-port guards. It is based on current @cjagwani, are you planning to refresh #7244 onto |
|
Closing this draft history as non-viable for the release. #7449 replaced only the closed EXDEV base #7243; it did not supersede the unique Hermes rebuild-bootstrap performance idea. However, this PR is still stacked on #7243’s closed, unmerged non-main branch, cannot be maintainer-edited, and its exact-head review has three HIGH route/port/binary-provenance defects plus incomplete credential scrubbing. The credentialed rebuild lanes never ran, so static checks cannot validate the replacement bootstrap. Please preserve the idea under #7140 and, only after its current prerequisites/design are settled, open a fresh current-main PR that configures and proves the inference route, separates API/dashboard ports, binds the installed OpenShell binary, scrubs both GitHub token variables with negative boundary tests, and runs both credentialed rebuild lanes. |
5 participants
Summary
Removes the throwaway current Hermes install/onboard/delete cycle from the rebuild E2E lanes. The rebuild scenarios now build the current source CLI in workspace setup, install pinned OpenShell, start the named gateway, and enter the old-sandbox rebuild fixture directly, saving an estimated 6–7 minutes per lane while retaining backup/recreate/restore and stale-base coverage.
Related Issue
Part of #7140.
Changes
prepare-e2eCLI build and a credential-sanitized pinned OpenShell install in both Hermes rebuild jobs.install.shand its generated current sandbox with a direct current-CLI/pinned-gateway bootstrap.Type of Change
Quality Gates
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailablenpm 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: Charan Jagwani cjagwani@nvidia.com