ci(e2e): select OpenShell 0.0.106 - #9378
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 skippedAuto reviews are disabled on base/target branches other than the default branch. 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:
Comment |
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
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 for the commit under review. Recommended E2E: None Manual-only E2E: 3 optional E2E recommendations
1 warning · 0 suggestionsWarningsWarnings do not block.
|
9de04aa to
fd6f90e
Compare
Prerequisite-PR: NVIDIA#9378 Prerequisite-Commit: fd6f90e Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
jyaunches
left a comment
There was a problem hiding this comment.
Could this migration keep scripts/install-openshell.sh as the one installer? The new tools/e2e/install-openshell-v00106-qualification.sh repeats the existing installer OS and architecture checks, archive validation, download, installation, and version verification in 84 lines. The workflow already used scripts/install-openshell.sh with NEMOCLAW_OPENSHELL_PIN_VERSION. Adding the reviewed 0.0.106 pins to that owner would remove the parallel installer while leaving the default supported pin at 0.0.101.
Please also consider one declarative 0.0.106 identity record for the version, source commit SHA, archive hashes, binary hashes, and supervisor digest. The current diff copies parts of that identity across the new installer, tools/e2e/mcp-workflow-boundary.mts, .github/workflows/e2e.yaml, and tests. expectedOpenShellE2eVersion then adds another hard-coded transition rule. A shared record consumed by installation, provenance generation, and boundary checks would reduce lines of code and make the next version change update one source. It would also let the two nearly identical provenance JSON blocks use one helper.
This is a simplicity and refactoring suggestion. I did not find missing validation in the current path.
There was a problem hiding this comment.
LOC Reduction / Codebase Simplicity Review
Why this blocks
tools/e2e/install-openshell-v00106-qualification.shduplicates the existing installer path in about 84 lines.- The 0.0.106 version, source SHA, archive digest, binary digests, and supervisor digest are repeated across the installer, workflow boundary, workflow, and tests.
Refactor direction
- Keep
scripts/install-openshell.shas the one installer and add the reviewed 0.0.106 pins to its existing version owner. - Retain 0.0.101 as the supported default.
- Define one declarative 0.0.106 identity record and consume it from installation, provenance generation, and boundary checks.
- Reuse one helper for the two provenance JSON blocks.
Expected result
- Delete the parallel qualification installer.
- Reduce hard-coded transition rules and make the next OpenShell update change one source of truth.
jyaunches
left a comment
There was a problem hiding this comment.
LOC Reduction / Codebase Simplicity Review\n\nRequesting changes because this PR adds a second OpenShell installer and repeats one provenance record across the installer, workflow boundary, workflow, and tests. Reuse �[0;32m[install]�[0m Detected macOS (aarch64)
�[0;32m[install]�[0m Installing OpenShell from release 'v0.0.101'...
�[0;32m[install]�[0m Downloading OpenShell Homebrew formula...
�[0;32m[install]�[0m staging Homebrew formula in tap nvidia/openshell...
�[0;32m[install]�[0m installing OpenShell with Homebrew...
==> Would install 1 formula:
openshell
==> Installing openshell from nvidia/openshell, add the reviewed 0.0.106 pins to its existing version owner while retaining 0.0.101 as the supported default, and generate the version, source SHA, archive digest, binary digests, and supervisor digest from one declarative record. This should remove and the duplicated provenance JSON without weakening qualification.
Superseded by the correctly rendered structured LOC Reduction / Codebase Simplicity Review.
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Summary
The trusted full-E2E workflow qualifies OpenShell 0.0.106 while the supported NemoClaw product pin remains 0.0.101. This prerequisite now uses the product installer plus one declarative qualification identity for the reviewed 0.0.106 CLI, gateway, sandbox, and supervisor runtime.
Changes
scripts/install-openshell.shas the sole installer implementation.scripts/install-openshell.sh; the supported default remains 0.0.101.Type of Change
Quality Gates
e1166f5c3. Credential variables are removed before installation, candidate-controlled shell generation is absent, workflows invoke one checked-in installer, fixed SHA-256 and OCI digest identities bind 0.0.106, the base verifier checks both cohorts, and product defaults and authorization boundaries are unchanged.Documentation Writer Review
no-docs-neededDGX Station Hardware Evidence
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed — justification: The clean repair worktree could not resolve local TypeScript packages, so the repair commits and push bypassed hooks. GitHub Actions is running these checks.git diff --checkpassed for the behavior correction and the final comment correction; the synchronization commit is empty. Shell formatting, ShellCheck, YAML, secret scanning, and structural checks passed. Dependency-backed local checks did not run because the clean worktree has no installed packages.e1166f5c3with stacked base verifier commit6fb04d3eb.npm run docsbuilds without warnings (doc changes only)Signed-off-by: Prekshi Vyas prekshiv@nvidia.com
Signed-off-by: Carlos Villela cvillela@nvidia.com