fix(onboard): bound compatible endpoint probe#5400
Merged
Conversation
This reverts commit 3f003c0.
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Contributor
📝 WalkthroughWalkthroughThis PR replaces the hosted inference model identifier with ChangesHosted Inference Model Migration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in the branch is 96%. Coverage data for the branch is not yet available. Show a code coverage summary of the most covered files.
TypeScript / code-coverage/cliThe overall coverage in the branch is 44%. Coverage data for the branch is not yet available. Show a code coverage summary of the most covered files.
Updated |
Contributor
E2E Advisor RecommendationRequired E2E: Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
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 AdvisorFindings: 0 needs attention, 0 worth checking, 0 nice ideas Consider writing more tests for
This is an automated advisory review. A human maintainer must make the final merge decision. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/test-rebuild-hermes.sh`:
- Around line 287-288: Update the seeded registry model ID to the migrated
served value in all three E2E scripts: in test/e2e/test-rebuild-hermes.sh (lines
287-288) replace the registry 'model' value with
'nvidia/nvidia/nemotron-3-super-v3' (leave 'provider' as-is), in
test/e2e/test-rebuild-openclaw.sh (lines 222-223) replace the registry 'model'
value with 'nvidia/nvidia/nemotron-3-super-v3', and in
test/e2e/test-upgrade-stale-sandbox.sh (lines 162-163) replace the registry
'model' value with 'nvidia/nvidia/nemotron-3-super-v3' so all seeded registry
entries use the served provider model ID.
🪄 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: c16d651b-d919-4a3d-9591-56d9f2f1d36d
📒 Files selected for processing (12)
.github/workflows/e2e-script.yaml.github/workflows/e2e-vitest-scenarios.yaml.github/workflows/nightly-e2e.yamlsrc/lib/inference/onboard-probes.test.tssrc/lib/inference/onboard-probes.tssrc/lib/onboard/providers.tstest/e2e-scenario/fixtures/hosted-inference.tstest/e2e-script-workflow.test.tstest/e2e/lib/ci-compatible-inference.shtest/e2e/test-rebuild-hermes.shtest/e2e/test-rebuild-openclaw.shtest/e2e/test-upgrade-stale-sandbox.sh
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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-script-workflow.test.ts`:
- Line 926: Replace the CWD-dependent read with a module-relative read: when
reading the fixture use readFileSync(path.join(__dirname, fixture), "utf8") (or
readFileSync(new URL(fixture, import.meta.url), "utf8") in ESM) instead of
readFileSync(fixture, "utf8"); update the import/require to include path (or
ensure URL usage) so the assignment to body uses a module-relative path derived
from __dirname or import.meta.url and no longer depends on the process CWD.
🪄 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: 6377e0da-6fdc-47a7-ba94-65e9f9892c3d
📒 Files selected for processing (4)
test/e2e-script-workflow.test.tstest/e2e/test-rebuild-hermes.shtest/e2e/test-rebuild-openclaw.shtest/e2e/test-upgrade-stale-sandbox.sh
| @@ -925,9 +925,10 @@ describe("E2E reusable workflow contract", () => { | |||
| for (const fixture of rebuildFixtures) { | |||
| const body = readFileSync(fixture, "utf8"); | |||
Contributor
There was a problem hiding this comment.
Use module-relative reads for rebuild fixtures to avoid CWD-coupled test failures.
readFileSync(fixture, "utf8") relies on the test runner’s current working directory. This can flake when the suite is invoked from a different CWD.
Suggested patch
- for (const fixture of rebuildFixtures) {
- const body = readFileSync(fixture, "utf8");
+ for (const fixture of rebuildFixtures) {
+ const body = readFileSync(new URL(`../${fixture}`, import.meta.url), "utf8");
expect(body, fixture).toContain("provider = sess.get('provider')");
expect(body, fixture).toContain("if env_provider == 'custom'");
expect(body, fixture).toContain("'provider': provider");
expect(body, fixture).toContain("'model': model");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const body = readFileSync(fixture, "utf8"); | |
| const body = readFileSync(new URL(`../${fixture}`, import.meta.url), "utf8"); |
🤖 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-script-workflow.test.ts` at line 926, Replace the CWD-dependent read
with a module-relative read: when reading the fixture use
readFileSync(path.join(__dirname, fixture), "utf8") (or readFileSync(new
URL(fixture, import.meta.url), "utf8") in ESM) instead of readFileSync(fixture,
"utf8"); update the import/require to include path (or ensure URL usage) so the
assignment to body uses a module-relative path derived from __dirname or
import.meta.url and no longer depends on the process CWD.
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
1 participant
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
Reverts the hosted custom inference model-ID changes from #5399 so CI continues using the model ID actually served by
https://inference-api.nvidia.com/v1/chat/completions. Bounds the ordinary OpenAI-compatible chat-completions onboarding validation probe withmax_tokens: 8, and keeps rebuild/upgrade E2E registry metadata aligned with the hosted-compatible onboarding session.Changes
nvidia/nvidia/nemotron-3-super-v3.nvidia-prodmodel ID.max_tokens: 8to the non-strict chat-completions validation probe payload.Type of Change
Verification
npx prek run --from-ref main --to-ref HEADpassesnpm testpasses (broad runtime changes only)npm run docsbuilds without warnings (doc changes only)Notes:
npx prek run --from-ref main --to-ref HEADpassed before the latest fixture update; commit and push hooks passed for the latest update.bash -npassed for the changed rebuild/upgrade shell fixtures.npm test -- src/lib/inference/onboard-probes.test.ts test/e2e-script-workflow.test.ts src/lib/onboard/providers.test.tspassed.npm test -- test/onboard-selection.test.ts test/stale-dist-check.test.ts src/lib/inference/onboard-probes.test.tspassed.npm test -- test/onboard-model-router.test.ts -t "prefers the managed Model Router command over PATH"passed after one transient commit-hook failure in that unrelated test.npm run docspassed with 0 errors; Fern reported 2 hidden warnings, so the docs-without-warnings checkbox is left unchecked.Signed-off-by: Carlos Villela cvillela@nvidia.com
Summary by CodeRabbit
Chores
Bug Fixes
Tests