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, and the Hermes E2E test uses the adapter for environment 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 |
deepujain
force-pushed
the
test/5745-e2e-inference-adapter
branch
from
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. |
prekshivyas
force-pushed
the
test/5745-e2e-inference-adapter
branch
from
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. |
deepujain
force-pushed
the
test/5745-e2e-inference-adapter
branch
from
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>
deepujain
force-pushed
the
test/5745-e2e-inference-adapter
branch
from
b7884e2 to
129ffb1
Compare
|
/ok to test 51d4309 |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/e2e.yaml (1)
1710-1715: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winIn
.github/workflows/e2e.yaml:1710-1715, keepNVIDIA_INFERENCE_API_KEYout of PR-checkout runs.hermes-e2estill checks outinputs.checkout_shaand runs Vitest with the key wheneverinference_modeis non-mock, so PR code can read and exfiltrate it. Gate the secret on a trusted-only path or move the live inference call behind a trusted service.🤖 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 @.github/workflows/e2e.yaml around lines 1710 - 1715, Update the hermes-e2e workflow environment setup so NVIDIA_INFERENCE_API_KEY is only exposed on trusted, non-PR checkout runs; do not provide the secret when executing code checked out via inputs.checkout_sha. Preserve the existing mock-mode behavior and live inference execution, using the workflow’s trusted-run context to gate secret availability.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.
Outside diff comments:
In @.github/workflows/e2e.yaml:
- Around line 1710-1715: Update the hermes-e2e workflow environment setup so
NVIDIA_INFERENCE_API_KEY is only exposed on trusted, non-PR checkout runs; do
not provide the secret when executing code checked out via inputs.checkout_sha.
Preserve the existing mock-mode behavior and live inference execution, using the
workflow’s trusted-run context to gate secret availability.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d5e7b7df-5035-48a1-b147-7b0036d79ff7
📒 Files selected for processing (1)
.github/workflows/e2e.yaml
PR Review Advisor — InformationalAdvisor assessment: Informational / medium confidence Model lanes
Nemotron is a non-blocking second opinion. Its prose, findings, and E2E guidance do not change the primary assessment above and remain in workflow artifacts only. E2E guidanceAdvisory only: coverage and selector recommendations are non-authoritative. E2E / PR Gate independently computes and dispatches trusted jobs without consuming this output. Recommended coverage:
This is an automated, non-authoritative review. Findings are inputs to maintainer adjudication. Warnings and optional suggestions do not require a response or follow-up. A human maintainer makes the final merge decision. |
|
/ok to test 650edaf |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
/ok to test d9e02c1 |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
|
/ok to test 3ca03ac |
✅ Action performedReview finished.
|
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/live/hermes-e2e.test.ts`:
- Around line 432-444: Update the deniedEgress output assertion in the phase-3
egress test to accept curl’s standard 403 message (“The requested URL returned
error: 403”) alongside the existing denial patterns. Preserve the nonzero
exit-code assertion and current case-insensitive matching.
🪄 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: 05be3b1d-0952-4fef-8b98-1aed1d755180
📒 Files selected for processing (1)
test/e2e/live/hermes-e2e.test.ts
## Summary - replace the unsupported TypeScript constructor parameter property in the PR E2E gate - add a regression that launches the controller with the exact `node --experimental-strip-types` runtime used by Actions ## Why Current `main` crashes every PR E2E gate during initialization with `ERR_UNSUPPORTED_TYPESCRIPT_SYNTAX`, before it can evaluate the PR. This is visible on #6770, #6711, and #6672. ## Validation - `npx vitest run --project integration test/pr-e2e-gate-command.test.ts test/pr-e2e-gate-lifecycle.test.ts test/pr-e2e-gate-exceptions.test.ts test/pr-e2e-gate.test.ts test/pr-e2e-gate-workflow.test.ts` (108 passed) - `npm run build:cli` - pre-commit and pre-push hooks <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved validation for invalid end-to-end gate modes, including clearer error messaging and the correct failure status. * Prevented unsupported TypeScript syntax errors when running the gate command directly in Node’s experimental type-stripping mode. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
|
@coderabbitai review |
|
/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 |
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
npm install --ignore-scripts- completed.npm --prefix nemoclaw install --ignore-scripts- completed.npm run build:cli- passed.npm run source-shape:check- passed.npm run typecheck:cli- passed.npm run lint- passed.npm run --silent test -- --project e2e-support test/e2e/support/inference-adapter.test.ts- passed, 5 tests.git diff --check upstream/main...HEAD- passed.npm test- attempted; local run reached the broad Vitest suite and hit my command timeout rather than a branch assertion failure.Refs #5745
Signed-off-by: Deepak Jain deepujain@gmail.com
Summary by CodeRabbit
New Features
inference_modeselector (mock,internal-nvidia,public-nvidia) to E2E workflow dispatch (default:mock) and standardized inference selection viaNEMOCLAW_E2E_INFERENCE_MODE.Tests