fix(e2e): correct full release gate lanes - #5917
Merged
Merged
Conversation
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughUpdates model-router provider-routed inference to use ChangesE2E workflow and scenario updates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in the Show a code coverage summary of the most covered files.
TypeScript / code-coverage/cliThe overall coverage in the Show a code coverage summary of the most covered files.
Updated |
Contributor
E2E Advisor RecommendationRequired E2E: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
|
Contributor
Vitest E2E Scenario RecommendationRequired Vitest E2E scenarios: Dispatch required Vitest E2E scenarios:
Full Vitest E2E advisor summaryVitest E2E Scenario AdvisorBase: Required Vitest E2E scenarios
Optional Vitest E2E scenarios
Relevant changed files
|
Contributor
PR Review Advisor (Nemotron Ultra) — Changes requestedMerge posture: Do not merge yet Action checklist
Findings index
🚨 Required before mergeAddress these before merging unless a maintainer explicitly overrides the advisor with rationale.
|
Contributor
PR Review Advisor — Changes requestedMerge posture: Do not merge yet Action checklist
Findings index
Review findings by urgency: 0 required fixes, 2 items to resolve/justify, 0 in-scope improvements
|
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/e2e-scenario/support-tests/model-router-provider-routed-inference.test.ts (1)
28-34: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winLock in the negative credential-boundary case.
This only proves the aliases are added. Because
baseEnvis{}, it would still pass if the helper also forwarded the rawNVIDIA_API_KEYinto the child env, which is the drift this PR is trying to prevent.Suggested test tightening
it("stages the public key under the credential names consumed by the router", () => { - expect(buildProviderRoutedEnv("nvapi-public-test-key", "e2e-router", {})).toMatchObject({ + const env = buildProviderRoutedEnv("nvapi-public-test-key", "e2e-router", { + NVIDIA_API_KEY: "nvapi-public-test-key", + }); + expect(env).toMatchObject({ NVIDIA_INFERENCE_API_KEY: "nvapi-public-test-key", NEMOCLAW_PROVIDER_KEY: "nvapi-public-test-key", NEMOCLAW_PROVIDER: "routed", NEMOCLAW_SANDBOX_NAME: "e2e-router", }); + expect(env).not.toHaveProperty("NVIDIA_API_KEY"); });As per path instructions, tests here should “Prefer observable outcomes through the public boundary...”, and the PR objective explicitly calls out staging under
NVIDIA_INFERENCE_API_KEY.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e-scenario/support-tests/model-router-provider-routed-inference.test.ts` around lines 28 - 34, The test for buildProviderRoutedEnv only checks the expected aliases, so it can still pass even if the raw NVIDIA_API_KEY leaks into the routed env. Tighten the negative credential-boundary coverage in model-router-provider-routed-inference.test.ts by asserting that the result does not include NVIDIA_API_KEY when staging the public key through buildProviderRoutedEnv, while still verifying the existing aliases and sandbox fields.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@test/e2e-scenario/support-tests/model-router-provider-routed-inference.test.ts`:
- Around line 28-34: The test for buildProviderRoutedEnv only checks the
expected aliases, so it can still pass even if the raw NVIDIA_API_KEY leaks into
the routed env. Tighten the negative credential-boundary coverage in
model-router-provider-routed-inference.test.ts by asserting that the result does
not include NVIDIA_API_KEY when staging the public key through
buildProviderRoutedEnv, while still verifying the existing aliases and sandbox
fields.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 00323f86-91ba-4902-8b46-265d6a9263f5
📒 Files selected for processing (8)
.github/workflows/e2e-vitest-scenarios.yaml.github/workflows/regression-e2e.yamltest/e2e-scenario/live/model-router-provider-routed-inference-helpers.tstest/e2e-scenario/live/model-router-provider-routed-inference.test.tstest/e2e-scenario/support-tests/model-router-provider-routed-inference.test.tstest/e2e/test-model-router-provider-routed-inference.shtest/regression-e2e-workflow.test.tstools/e2e-scenarios/workflow-boundary.mts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/e2e-scenario/support-tests/hermes-inference-switch-command-shape.test.ts (1)
12-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise the probe behavior, not the exact argv.
This locks the test to one
grepspelling, while a semantically equivalent command would fail and a broken pattern can still pass. Please add fixture cases that prove match/non-match behavior for representative config lines instead of snapshotting the exact array. As per path instructions, “Prefer observable outcomes through the public boundary over source-text, private-shape, or mock-call assertions.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e-scenario/support-tests/hermes-inference-switch-command-shape.test.ts` around lines 12 - 16, The test currently asserts the exact argv returned by apiKeyShapeCommand, which over-constrains the implementation instead of verifying the probe behavior. Update hermes-inference-switch-command-shape.test.ts to exercise observable match/non-match outcomes using fixture config lines for representative cases, and keep the assertions focused on whether the API-key probe succeeds or fails rather than the specific grep arguments. Use apiKeyShapeCommand and the probe behavior as the entry points, and remove the direct snapshot-style array expectation.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/e2e-scenario/live/hermes-inference-switch-helpers.ts`:
- Around line 350-353: The apiKeyShapeCommand probe uses API_KEY_SHAPE_PATTERN
in hermes-inference-switch-helpers.ts, and the regex is too loose because it
matches only the prefix of the YAML scalar. Tighten API_KEY_SHAPE_PATTERN so it
anchors the entire api_key value through the end of the scalar, preventing
trailing junk or unterminated quotes from passing the grep check. Update the
pattern used by apiKeyShapeCommand to require a full-line match for the intended
YAML value shape.
---
Nitpick comments:
In
`@test/e2e-scenario/support-tests/hermes-inference-switch-command-shape.test.ts`:
- Around line 12-16: The test currently asserts the exact argv returned by
apiKeyShapeCommand, which over-constrains the implementation instead of
verifying the probe behavior. Update
hermes-inference-switch-command-shape.test.ts to exercise observable
match/non-match outcomes using fixture config lines for representative cases,
and keep the assertions focused on whether the API-key probe succeeds or fails
rather than the specific grep arguments. Use apiKeyShapeCommand and the probe
behavior as the entry points, and remove the direct snapshot-style array
expectation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: dce55dae-ed3a-4ce1-bc40-856911d09372
📒 Files selected for processing (7)
ci/test-file-size-budget.jsonnemoclaw-blueprint/model-specific-setup/openclaw/nemotron-3-ultra-managed-inference.jsontest/e2e-scenario/live/hermes-inference-switch-helpers.tstest/e2e-scenario/live/network-policy.test.tstest/e2e-scenario/support-tests/hermes-inference-switch-command-shape.test.tstest/e2e/test-network-policy.shtest/generate-openclaw-config.test.ts
✅ Files skipped from review due to trivial changes (3)
- nemoclaw-blueprint/model-specific-setup/openclaw/nemotron-3-ultra-managed-inference.json
- ci/test-file-size-budget.json
- test/e2e/test-network-policy.sh
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Contributor
Vitest E2E Scenario Results — ❌ Some jobs failedRun: 28307218499
|
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 28307219283
|
Collaborator
Author
|
Advisor follow-up for head
The isolated network-policy validation is still running; I will record its final result before merge. |
Collaborator
Author
|
Credential-name clarification for the release-gate record:
The Model Router lane now sources only |
Contributor
Vitest E2E Scenario Results — ❌ Some jobs failedRun: 28307373397
|
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Collaborator
Author
|
Network-policy follow-up: isolated run https://github.com/NVIDIA/NemoClaw/actions/runs/28307373397 reached the real Slack assertion and proved the endpoint change alone was insufficient. Artifacts showed Head |
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Contributor
Vitest E2E Scenario Results — ✅ All requested jobs passedRun: 28307920824
|
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Collaborator
Author
|
Final advisor follow-up:
Runtime |
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 28308152912
|
Collaborator
Author
|
Final live/security evidence for head
The final merged-main full gates will cover the intervening #5916 onboarding refactor before release. |
Collaborator
Author
|
Maintainer acceptance/override record for the final Nemotron review on
These findings are accepted for merge once required CI is green. |
cv
enabled auto-merge (squash)
June 28, 2026 02:14
cv
disabled auto-merge
June 28, 2026 02:35
Hadar301
pushed a commit
to Hadar301/NemoClaw-OpenShift
that referenced
this pull request
Jul 12, 2026
<!-- markdownlint-disable MD041 --> ## Summary Correct the deterministic failures exposed by the full nightly/Vitest release gate. The fixes cover credential and project selection, hosted Ultra tool compatibility, a stable Slack policy probe, and OpenShell's single-line command boundary. ## Changes - pass `NVIDIA_API_KEY` to the Model Router workflow and stage it under the router's `NVIDIA_INFERENCE_API_KEY` credential boundary - mirror the public-key fix in the retained regression Bash lane and add behavioral support coverage for key selection and staging - run `test/gateway-drift-preflight.test.ts` through the `integration` Vitest project - extend the workflow boundary validator so both command and credential contracts fail closed on future drift - disable OpenClaw native Tool Search for hosted Nemotron 3 Ultra so it uses structured tool calls instead of invalid generated JavaScript - probe the Slack preset through the non-redirecting `slack.com/api/api.test` endpoint in both Bash and Vitest lanes, using prompt-synchronized `expect` and an applied-policy assertion - pass the Hermes API-key shape check as direct single-line argv accepted by current OpenShell, with support coverage ## Type of Change - [x] 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) ## Quality Gates - [x] Tests added or updated for changed behavior - [x] Existing tests cover changed behavior — justification: the existing gateway-drift integration test and workflow-boundary suite exercise the corrected project and workflow contract - [ ] Tests not applicable — justification: - [ ] Docs updated for user-facing behavior changes - [x] Docs not applicable — justification: release-gate workflow and test-harness behavior only - [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: the public key remains step-scoped, the live helper stages only the credential aliases consumed by Model Router, and boundary tests reject hosted-key regression. The `expect` trust boundary is explicitly accepted: it is an existing reviewed host tool in the retained Bash lane; the Vitest lane installs it only from Ubuntu apt in trusted workflow YAML, feeds it only a regex-constrained numeric menu index plus literal `Y`, and enforces that shape with support and workflow-boundary tests. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Verification - [x] PR description includes the DCO sign-off declaration and every commit appears as `Verified` in GitHub - [x] Git hooks passed during commit and push, or `npx prek run --from-ref main --to-ref HEAD` passes - [x] Targeted tests pass for changed behavior - [ ] Full `npm test` passes (broad runtime changes only) - [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) --- Signed-off-by: Carlos Villela <cvillela@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added a Nemotron 3 Ultra managed inference route with native tool search disabled. * **Bug Fixes** * Updated routed-inference E2E credential wiring and secret redaction to use `NVIDIA_API_KEY` (`nvapi-*`) instead of `NVIDIA_INFERENCE_API_KEY`. * Updated network-policy Slack connectivity checks to target `https://slack.com/api/api.test`. * **Tests** * Expanded router/gateway-drift preflight Vitest coverage (correct env staging and integration project selection). * Added/strengthened Hermes and network-policy interactive preset parsing/expect sequencing tests. * Added a regression assertion for workflow env handling. * **Chores** * Improved CI workflow validation and ensured host dependency installation sequencing; adjusted a test file size budget. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Carlos Villela <cvillela@nvidia.com>
wscurran
added
area: ci
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Correct the deterministic failures exposed by the full nightly/Vitest release gate. The fixes cover credential and project selection, hosted Ultra tool compatibility, a stable Slack policy probe, and OpenShell's single-line command boundary.
Changes
NVIDIA_API_KEYto the Model Router workflow and stage it under the router'sNVIDIA_INFERENCE_API_KEYcredential boundarytest/gateway-drift-preflight.test.tsthrough theintegrationVitest projectslack.com/api/api.testendpoint in both Bash and Vitest lanes, using prompt-synchronizedexpectand an applied-policy assertionType of Change
Quality Gates
expecttrust boundary is explicitly accepted: it is an existing reviewed host tool in the retained Bash lane; the Vitest lane installs it only from Ubuntu apt in trusted workflow YAML, feeds it only a regex-constrained numeric menu index plus literalY, and enforces that shape with support and workflow-boundary tests.Verification
Verifiedin GitHubnpx prek run --from-ref main --to-ref HEADpassesnpm testpasses (broad runtime changes only)npm run docsbuilds without warnings (doc changes only)Signed-off-by: Carlos Villela cvillela@nvidia.com
Summary by CodeRabbit
NVIDIA_API_KEY(nvapi-*) instead ofNVIDIA_INFERENCE_API_KEY.https://slack.com/api/api.test.