fix(e2e): install reviewed SDK from locked dependencies - #11455
Conversation
Signed-off-by: San Dang <sdang@nvidia.com>
|
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. |
|
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 (7)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. 📝 WalkthroughWalkthroughThe E2E workflows now cache the SDK archive offline, run credential-free ChangesOffline SDK installation
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: Merge Risk: ⚪ Minimal · up to No actionable merge-blocking risk remains from the reviewed SDK installation changes. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 4 files. (3 skipped: 3 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall line coverage in commit 4238cdd in the TypeScript / code-coverage/cliThe overall line coverage in commit 4238cdd in the Updated |
prekshivyas
left a comment
prekshivyas
left a comment
There was a problem hiding this comment.
Reviewed exact head 4238cdd with no blocking findings. The change replaces unlocked SDK peer re-resolution with lockfile-driven npm ci, stages the reviewed archive in npm cache without package credentials, keeps lifecycle scripts disabled, and validates the SDK connection API. Independent Linux/Brev verification passed all 8 focused SDK-install tests, including real offline installs for both workflow paths, locked transitive/sibling dependencies, missing or ambiguous archives, and failure propagation. This directly addresses the 61-job npm Arborist edgesOut signature in main run 34501169319. Final live validation remains the first trusted main E2E run after merge.
wscurran
added
area: ci
## Outcome Manual PR E2E now stages the active reviewed OpenShell SDK and any approved transition replacement, then lets the candidate lockfile select the exact version. This preserves ordinary main runs while allowing reviewed SDK upgrade PRs to reach product tests. ## Reason PR #11251 run 34540154148 failed across the catalogue before product execution. The trusted workflow packaged SDK 0.0.106 from main, while the candidate lock requires 0.0.116. After #11455 switched installation to cache staging plus npm ci, npm correctly followed the candidate lock but could not satisfy it from the single cached archive, omitted the private optional dependency, and every consumer failed with ERR_MODULE_NOT_FOUND. #11397 introduced the single-main-version artifact assumption. #11455 exposed that latent mismatch deterministically. ## Changes - Add an optional reviewed-replacement packaging mode without weakening the existing strict transition mode used by pull_request_target. - Make E2E package the active SDK plus an available base-approved replacement. - Allow exactly one archive in steady state or two during an approved transition, cache each without credentials or lifecycle scripts, and retain lockfile-driven npm ci. - Add real-npm coverage for both active and replacement lock selection in catalogue and external-gateway workflows. - Extend workflow and package-verifier boundary coverage and documentation. ## Verification - Exact retained PR artifact from run 34536766601 contained reviewed SDK 0.0.106 and 0.0.116 archives. Against PR #11251 package.json and package-lock.json, the proposed commands installed 509 packages, selected exactly SDK 0.0.116, and imported OpenShellClient.connect. - Real Linux npm tests cover active and replacement selection in both workflow scripts: 11/11 passed. - Package transition tests: 5/5 passed. - Standard-profile boundary tests: 17/17 passed. - Base-image, operations, watch-trigger, and workflow-plan coverage: 320 tests passed initially; the 19 missing-esbuild setup failures were repaired, then 106/107 workflow-plan tests passed and the sole 5-second container timeout passed with a 15-second harness budget. - Repository checks, source-shape checks, canonical CLI build, TypeScript checks, formatting, lint, YAML validation, secret scan, E2E semantic phases, growth guardrails, commit hooks, and pre-push hooks passed. - Brev was attempted first but its local auth-token JSON is truncated; a credential-free feedback report was submitted. The equivalent Linux Docker reproduction completed locally. Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * SDK packaging and installation now support one or two approved archive versions, including available transition replacements. * Optional replacement packages can be included when available, while required replacements are validated explicitly. * **Bug Fixes** * Archive discovery is now deterministic and rejects missing, excessive, or ambiguous archive combinations. * Reviewed SDK archives are cached offline without package credentials. * Dependency installation verifies the selected SDK version and suppresses lifecycle scripts for safer setup. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
3 participants
Outcome
Catalogue E2E jobs and the external-gateway health job install the reviewed OpenShell SDK from the locked dependency graph. SDK setup no longer re-resolves project dependencies before the tests run.
Reason
The gateway job failed during the SDK install added by #11397. At inspection time, 61 jobs in that run failed at the same shared step. The
--package-lock=falseinstall reproduced an npm ArboristedgesOutcrash while resolving Vitest peers, before any gateway test ran.Changes
npm ciwith lifecycle scripts disabled and cached packages preferred. Apply this to both SDK installation paths.Verification
edgesOutcrash with Node 22.23.2 and npm 10.9.8 in Linux ARM64 using the retained, integrity-verified SDK archive.npx vitest run --project e2e-support test/e2e/support/openshell-sdk-install.test.ts test/e2e/support/standard-profile-workflow-boundary.test.ts test/e2e/support/base-image-publication-workflow-boundary.test.ts test/e2e/support/workflow-plan.test.ts test/e2e/support/e2e-operations-workflow-boundary.test.ts --coverage=false— 255 tests passed. The final SDK test rerun passed all 8 tests.npx vitest run --project integration test/repository/vitest-watch-triggers.test.ts --coverage=false— 77 tests passed.npm run checks:repository,npm run source-shape:check, changed-file Oxlint/Oxfmt, ShellCheck for both install scripts, andgit diff --check— passed.NODE_OPTIONS=--max-old-space-size=8192 npm run validate:pr— passed in the isolated Linux container after building the CLI and plugin. All pre-commit, commit-message, and applicable pre-push checks passed.Review notes
Self-review covers NVIDIA/NemoClaw commit
316b0a6606cff97147f2c4a78ec9b49cc1f51de7, including sensitive paths.github/workflows/e2e-standard-profile.yaml,.github/workflows/e2e.yaml, andtools/e2e/standard-profile-workflow-boundary.mts. Review checked archive selection, lockfile installation, credential removal, disabled scripts, failure propagation, sibling coverage, and regression evidence. Independent review of these paths is pending.Publication validation uses canonical base
5b82037acb402862bb590ef57cb4d4c608e35ce7. Its validation entry points, hook configuration, package manifests, and lockfiles match the candidate. The changed workflow validator expectations and watch mappings were reviewed separately. Validation runs in an offline container without host credentials, signing access, host mounts, or a Docker socket. Resolved validator files and the Node/npm executables are fingerprinted. This records the maintainer-requested publication evidence and does not claim independent approval or a CI waiver. GitHub CI and live E2E confirmation remain pending.Signed-off-by: San Dang sdang@nvidia.com
Summary by CodeRabbit
Bug Fixes
Tests