test(e2e): add inference adapter fixture (Refs #5745) - #6672
Conversation
|
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:
📝 WalkthroughWalkthroughThe E2E workflow now selects an inference mode and exposes it to tests. A shared adapter supports mock, internal NVIDIA, and public NVIDIA inference, while Hermes uses it for setup, probing, sandbox validation, and chat requests. ChangesE2E inference routing
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant E2EWorkflow
participant HermesE2ETest
participant E2EInferenceAdapter
participant ProviderClient
E2EWorkflow->>HermesE2ETest: set inference mode
HermesE2ETest->>E2EInferenceAdapter: create adapter fixture
E2EInferenceAdapter->>ProviderClient: probe models
ProviderClient-->>E2EInferenceAdapter: model response
HermesE2ETest->>E2EInferenceAdapter: directChat request
E2EInferenceAdapter->>ProviderClient: chat/completions request
ProviderClient-->>E2EInferenceAdapter: chat response
HermesE2ETest->>E2EInferenceAdapter: close()
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
372f83c to
d91ce3e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/fixtures/inference-adapter.ts`:
- Around line 138-149: Update the env method to preserve the internal inference
API selection: use "openai-completions" for mock mode, but use
hosted.env.NEMOCLAW_PREFERRED_API for internal-nvidia mode, consistent with
requireHostedInferenceConfig. Apply the same change to the corresponding second
environment construction block.
- Around line 218-222: Ensure the fake server is always closed when artifact
persistence fails: update close() to wrap requests collection and writeJson in
try/finally, calling fake.close() in the finally block. Also wrap post-start
setup in the relevant setup function around fake initialization,
requests/artifact persistence, and adapter creation so failures after the server
starts close fake before propagating.
- Around line 174-177: Ensure the fetch-based model probing branches reject
unsuccessful HTTP responses before parsing or returning JSON. Update the
relevant logic in the inference adapter, including the branch around the
model-list request and the additional fetch branches around the other endpoint
requests, to check response.ok and throw an error for 4xx/5xx responses so retry
behavior is preserved.
In `@test/e2e/live/hermes-e2e.test.ts`:
- Line 290: Replace the broad truthiness assertion on
inference.probeModels("phase-1-inference-models") with an assertion that the
resolved response contains the expected inference.model property, validating the
Phase 1 contract through the public probeModels boundary.
In `@test/e2e/support/inference-adapter.test.ts`:
- Around line 23-31: Remove the new conditional from the secrets() test helper
to satisfy the Codebase Growth Guardrails check. Preserve missing-secret failure
behavior using a nullish-coalescing throw expression, or relocate secrets() to a
shared non-test fixtures module so the test-file scan does not count it.
🪄 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: 7e969733-7b6a-4154-bed8-997b4d994b4c
📒 Files selected for processing (5)
.github/workflows/e2e.yamltest/e2e/fixtures/e2e-test.tstest/e2e/fixtures/inference-adapter.tstest/e2e/live/hermes-e2e.test.tstest/e2e/support/inference-adapter.test.ts
|
Tightened the inference adapter contracts: hosted mode now preserves the preferred API, mock fetches fail on non-2xx responses, fake servers close on artifact errors, and the Hermes probe checks the selected model. Focused e2e-support test, source-shape, and lint/checks pass. |
f410bee to
4ca4aa2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
test/e2e/fixtures/inference-adapter.ts (3)
22-22: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
E2E_INFERENCE_MODE_VALUESduplicates theE2EInferenceModeunion.The literal strings in
E2E_INFERENCE_MODE_VALUESmust be kept manually in sync with theE2EInferenceModetype. Deriving one from the other removes the drift risk if a mode is ever added/renamed.♻️ Proposed refactor
-export type E2EInferenceMode = "mock" | "internal-nvidia" | "public-nvidia"; +export const E2E_INFERENCE_MODE_VALUES = ["mock", "internal-nvidia", "public-nvidia"] as const; +export type E2EInferenceMode = (typeof E2E_INFERENCE_MODE_VALUES)[number];And remove the duplicate declaration near Line 341.
Also applies to: 341-345
🤖 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/fixtures/inference-adapter.ts` at line 22, Derive E2E_INFERENCE_MODE_VALUES from the E2EInferenceMode type so the allowed mode literals have a single source of truth. Remove the duplicate manually maintained declaration near the later configuration block, while preserving the existing runtime values and type usage.
105-120: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLong positional-string constructor is error-prone.
OpenAiCompatibleInferenceAdapter's constructor takes 10 positional parameters, several of the same type (model,endpointUrl,requestEndpointUrl,apiKey,preferredApiare allstring). The previously-fixedpreferredApi/apiKeymixup (per past review) is exactly the class of bug this shape invites. An options object would make call sites self-documenting and harder to misorder.🤖 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/fixtures/inference-adapter.ts` around lines 105 - 120, Refactor OpenAiCompatibleInferenceAdapter to accept a single named options object instead of positional constructor parameters, including model, endpointUrl, requestEndpointUrl, apiKey, preferredApi, providerClient, artifacts, fake, and mode. Update every constructor call site to use the corresponding property names, preserving the existing assignments and contractLabel behavior while eliminating any risk of swapping same-typed values.
99-279: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated provider-client request logic across the two adapter classes.
probeModels/directChatinOpenAiCompatibleInferenceAdapter(Lines 142-156, 169-184) andPublicNvidiaInferenceAdapter(Lines 241-254, 261-274) build nearly identicalrequestJsoncalls, differing only in the endpoint source (requestEndpointUrlvsendpointUrl) and artifact defaults. Extracting a shared helper (e.g.requestViaProvider(providerClient, url, opts)) would reduce duplication and the risk of the two paths drifting (e.g., only one gaining a header/timeout change).🤖 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/fixtures/inference-adapter.ts` around lines 99 - 279, The provider-client request construction is duplicated between OpenAiCompatibleInferenceAdapter and PublicNvidiaInferenceAdapter. Extract a shared helper for the common requestJson setup, parameterized by providerClient, endpoint URL, artifact name, request body, and any adapter-specific values, then reuse it from both probeModels and directChat while preserving their current endpoints, headers, timeouts, and artifact defaults.
🤖 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/fixtures/inference-adapter.ts`:
- Line 158: Update the raw fetch fallbacks in probeModels(), directChat(), and
the additional fetch paths around the referenced model/request handling to
enforce the same explicit timeout behavior as the providerClient branches. Use
the existing timeout configuration and abort mechanism rather than leaving fetch
requests unbounded, while preserving their current request and response
handling.
---
Nitpick comments:
In `@test/e2e/fixtures/inference-adapter.ts`:
- Line 22: Derive E2E_INFERENCE_MODE_VALUES from the E2EInferenceMode type so
the allowed mode literals have a single source of truth. Remove the duplicate
manually maintained declaration near the later configuration block, while
preserving the existing runtime values and type usage.
- Around line 105-120: Refactor OpenAiCompatibleInferenceAdapter to accept a
single named options object instead of positional constructor parameters,
including model, endpointUrl, requestEndpointUrl, apiKey, preferredApi,
providerClient, artifacts, fake, and mode. Update every constructor call site to
use the corresponding property names, preserving the existing assignments and
contractLabel behavior while eliminating any risk of swapping same-typed values.
- Around line 99-279: The provider-client request construction is duplicated
between OpenAiCompatibleInferenceAdapter and PublicNvidiaInferenceAdapter.
Extract a shared helper for the common requestJson setup, parameterized by
providerClient, endpoint URL, artifact name, request body, and any
adapter-specific values, then reuse it from both probeModels and directChat
while preserving their current endpoints, headers, timeouts, and artifact
defaults.
🪄 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: 62a7c85d-0061-4ba2-b87d-f1b7e85fba9b
📒 Files selected for processing (5)
.github/workflows/e2e.yamltest/e2e/fixtures/e2e-test.tstest/e2e/fixtures/inference-adapter.tstest/e2e/live/hermes-e2e.test.tstest/e2e/support/inference-adapter.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- test/e2e/fixtures/e2e-test.ts
- .github/workflows/e2e.yaml
- test/e2e/support/inference-adapter.test.ts
- test/e2e/live/hermes-e2e.test.ts
E2E Target Results — ✅ All requested tests passedRun: 29183585756
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
PR Review Advisor — No blocking findingsMerge posture: No blocking advisor findings This is an automated review. Required findings need action before merge. Warnings and optional suggestions do not require a response or follow-up. A human maintainer makes the final merge decision. |
PR Review Advisor (Nemotron Ultra) — No blocking findingsMerge posture: No blocking advisor findings This is an automated review. Required findings need action before merge. Warnings and optional suggestions do not require a response or follow-up. A human maintainer makes the final merge decision. |
4a13cfb to
38fb5c1
Compare
|
Pushed a follow-up for the adapter cleanup: mode literals now have one source, compatible adapter construction uses named options, and provider-client calls share one helper. Focused e2e-support tests, typecheck, source-shape, and lint/checks pass; refreshed PR checks are green. |
Refs NVIDIA#5745 Signed-off-by: Deepak Jain <deepujain@gmail.com> Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> Signed-off-by: Deepak Jain <deepujain@gmail.com>
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> Signed-off-by: Deepak Jain <deepujain@gmail.com>
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> Signed-off-by: Deepak Jain <deepujain@gmail.com>
Signed-off-by: Deepak Jain <deepujain@gmail.com>
Signed-off-by: Deepak Jain <deepujain@gmail.com>
Signed-off-by: Deepak Jain <deepujain@gmail.com>
Signed-off-by: Deepak Jain <deepujain@gmail.com>
Signed-off-by: Deepak Jain <deepujain@gmail.com>
b7884e2 to
129ffb1
Compare
|
/ok to test be24cdf |
✅ Action performedReview finished.
|
|
/ok to test d137277 |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Exact-head re-review request for Both requested P1s are addressed: mock Hermes receives no NVIDIA credential and launches its child with a minimal environment plus sentinel-secret coverage; route identity now parses state and compares the exact expected provider/model. The fixture also proves intended The remaining machine gate is the expected fork safety exception for credential-bearing E2E. Gate run 29305639898 is queued at |
|
New exact head after the main-only merge: The remaining policy gate requires a maintainer fork exception for this exact revision: controller run 29308380469. @cv @ericksoa, please approve the protected environment or run the |
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
|
/ok to test 9fc648e |
|
@coderabbitai review |
✅ Action performedReview finished.
|
E2E Target Results — ❌ Some tests failedRun: 29364133952
|
E2E Target Results — ✅ All requested tests passedRun: 29364476037
|
|
Exact-head completion summary for
The temporary trusted dispatch branch was deleted after completion. The only remaining red gate is the fork-policy exception: controller run 29364378789 is waiting for the protected environment reviewer. @cv, please approve the credentialed-E2E skip for this exact head/base. @cjagwani, please complete the requested re-review; no unresolved review threads remain. @ericksoa, the typed |
prekshivyas
left a comment
prekshivyas
left a comment
There was a problem hiding this comment.
Reviewed current head 9fc648e1. The credential boundary and exact route-identity fixes are sound; focused/local checks, standard and self-hosted CI, both advisors, CodeRabbit, and all advisor-selected exact-head E2E targets are clean (credential-sanitization passed on focused retry after the first attempt hit an external socket reset). LGTM pending the protected fork-policy gate approval.
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> # Conflicts: # test/e2e/support/e2e-workflow.test.ts # tools/e2e/workflow-boundary.mts
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
|
@cv Please approve the protected credentialed-E2E skip for fork PR #6672 at exact head The trusted controller selected |
5 participants
Summary
Adds a shared Vitest E2E inference adapter so live scenarios can choose between:
This is a focused pilot for #5745. It converts the Hermes live E2E path to the adapter without migrating every E2E scenario in one PR.
Changes
test/e2e/fixtures/inference-adapter.tswithmock,internal-nvidia, andpublic-nvidiamodes.test/e2e/fixtures/e2e-test.tsfixture graph.test/e2e/live/hermes-e2e.test.tsto use the adapter for provider env, model probe, direct chat, redaction values, and route assertions.nvapi-validation, and unknown-mode rejection.inference_modeto the consolidatedE2Eworkflow withmockas the default.Testing
Local verification
npm run checks- passed.npm run typecheck- passed.npm run build:cli- passed.npm run source-shape:check- passed.npm run test-conditionals:scan- passed.Exact-head CI and E2E (
9fc648e1e20fcc7496b8ca9723e52fa93ac113f4)cloud-onboard,security-posture,hermes-dashboard,hermes-e2e,inference-routing,network-policy, and optionalcloud-inferencepassed. The firstcredential-sanitizationattempt hit an external release-fetchsocket hang upbefore OpenShell installation.Refs #5745
Signed-off-by: Deepak Jain deepujain@gmail.com
Summary by CodeRabbit
New Features
Bug Fixes
Tests