Skip to content

fix(e2e): use hosted validation models in Vitest lanes - #5892

Closed
cv wants to merge 2 commits into
mainfrom
fix/vitest-hosted-validation-models
Closed

fix(e2e): use hosted validation models in Vitest lanes#5892
cv wants to merge 2 commits into
mainfrom
fix/vitest-hosted-validation-models

Conversation

@cv

@cv cv commented Jun 27, 2026

Copy link
Copy Markdown
Collaborator

Summary

Fixes hosted-compatible Vitest lanes that reached the custom provider path but still used public/provider-specific model or env setup, causing hosted endpoint validation to fail with HTTP 401/403. The affected tests now use the shared hosted-compatible model/env plumbing consistently.

Changes

  • Removes the Hermes Vitest workflow override for NEMOCLAW_MODEL=minimaxai/minimax-m2.7 so hosted-compatible runs use the shared hosted model default.
  • Updates Hermes and cron live Vitest tests to default to DEFAULT_HOSTED_INFERENCE_MODEL.
  • Routes diagnostics and stale-upgrade live Vitest setup through requireHostedInferenceConfig() / hosted.env instead of manually passing only NVIDIA_INFERENCE_API_KEY.

Type of Change

  • 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

  • Tests added or updated for changed behavior
  • Existing tests cover changed behavior — justification: live Vitest files are gated locally; TypeScript and hosted-inference support tests cover the shared env helper.
  • Tests not applicable — justification:
  • Docs updated for user-facing behavior changes
  • Docs not applicable — justification: live E2E harness behavior only.
  • Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging)
  • Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: self-review; change narrows test model/env selection to the existing hosted-compatible fixture boundary without broadening secret exposure.
  • Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue:

Verification

  • PR description includes the DCO sign-off declaration and every commit appears as Verified in GitHub
  • Git hooks passed during commit and push, or npx prek run --from-ref main --to-ref HEAD passes
  • Targeted tests pass for changed behavior
  • Full npm test passes (broad runtime changes only)
  • Quality Gates section completed with required justifications or waivers
  • No secrets, API keys, or credentials committed
  • npm run docs builds without warnings (doc changes only)
  • Doc pages follow the style guide (doc changes only)
  • New doc pages include SPDX header and frontmatter (new pages only)

Targeted verification:

npm run typecheck:cli

Live scenario test files are gated locally unless NEMOCLAW_RUN_E2E_SCENARIOS=1 is set; the validation evidence will come from targeted GitHub E2E reruns after merge.


Signed-off-by: Carlos Villela cvillela@nvidia.com

Summary by CodeRabbit

  • Bug Fixes
    • Improved reliability of end-to-end scenarios by standardizing how hosted inference settings are picked up across live tests.
    • Updated default model selection to fall back more consistently when no model is provided.
    • Removed a fixed model setting from one test workflow, reducing environment-specific failures.

Signed-off-by: Carlos Villela <cvillela@nvidia.com>
@cv cv self-assigned this Jun 27, 2026
@coderabbitai

coderabbitai Bot commented Jun 27, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Important

Review skipped

Review was skipped as selected files did not have any reviewable changes.

💤 Files selected but had no reviewable changes (1)
  • tools/e2e-scenarios/workflow-boundary.mts
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: e5bb6e02-d28d-4c83-a223-d715655f5664

📥 Commits

Reviewing files that changed from the base of the PR and between bb236f1 and 1fe54b4.

📒 Files selected for processing (1)
  • tools/e2e-scenarios/workflow-boundary.mts

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Live e2e scenario tests and the Hermes vitest workflow now use shared hosted-inference defaults and environment plumbing. Hardcoded model IDs and direct API-key extraction were replaced with DEFAULT_HOSTED_INFERENCE_MODEL, requireHostedInferenceConfig, and propagated hosted env values.

Changes

Hosted inference configuration plumbing

Layer / File(s) Summary
Model defaults and CI override removal
test/e2e-scenario/live/cron-preflight-inference-local.test.ts, test/e2e-scenario/live/diagnostics.test.ts, test/e2e-scenario/live/hermes-e2e.test.ts, .github/workflows/e2e-vitest-scenarios.yaml
Live scenario tests switch their fallback model to DEFAULT_HOSTED_INFERENCE_MODEL, and the Hermes vitest job no longer sets NEMOCLAW_MODEL.
Diagnostics hosted config wiring
test/e2e-scenario/live/diagnostics.test.ts
diagnostics.test.ts derives hosted inference config through requireHostedInferenceConfig, reads the API key from hosted.apiKey, and passes hosted.env into the cleanup environment.
Hermes command environment threading
test/e2e-scenario/live/hermes-e2e.test.ts
hermes-e2e.test.ts updates commandEnv to accept hosted env values, uses them for NEMOCLAW_MODEL, and passes hosted.env into the scenario setup.
Shared stale-sandbox helper contract
test/e2e-scenario/live/upgrade-stale-sandbox-helpers.ts
commandEnv merges extra env values into the probe env, installCurrentNemoclaw accepts hosted config, and host.command receives the hosted env plus hosted.apiKey redaction.
Stale-sandbox test wiring
test/e2e-scenario/live/upgrade-stale-sandbox.test.ts
The test loads hosted config from secrets and uses hosted.env and hosted.apiKey across install, upgrade checks, rebuild, and the final validation.

Sequence Diagram(s)

sequenceDiagram
  participant T as upgrade-stale-sandbox.test.ts
  participant C as requireHostedInferenceConfig
  participant H as installCurrentNemoclaw
  participant E as commandEnv
  participant S as host.command
  T->>C: read hosted.apiKey and hosted.env from secrets
  C-->>T: return hosted config
  T->>H: pass hosted
  H->>E: merge hosted.env into command env
  H->>S: run install with env and redactionValues
  T->>S: run upgrade and rebuild checks with commandEnv(hosted.env)
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

  • NVIDIA/NemoClaw#5891: Refactors the same hosted-inference env and commandEnv plumbing in live e2e tests.
  • NVIDIA/NemoClaw#5751: Also updates live scenario model defaults to a hosted inference model constant.
  • NVIDIA/NemoClaw#5776: Shares the same hosted-inference fixture and env wiring changes in e2e tests.

Suggested labels

area: ci, area: e2e, area: inference, bug-fix

Suggested reviewers

  • jyaunches

Poem

I hopped through the build with a hosted-dream grin,
New defaults fluttered where hardcodes had been.
My carrot-red keys stayed tucked out of sight,
And sandboxes danced through the night.
🐇🌙

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: switching Vitest e2e lanes to hosted-compatible validation models and environment plumbing.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/vitest-hosted-validation-models

Comment @coderabbitai help to get the list of available commands.

@github-code-quality

github-code-quality Bot commented Jun 27, 2026

Copy link
Copy Markdown
Contributor

Code Coverage Overview

Languages: TypeScript

TypeScript / code-coverage/plugin

The overall coverage in the fix/vitest-hosted-va... branch is 96%. Coverage data for the main branch is not yet available.

Show a code coverage summary of the most covered files.
File main fix/vitest-hosted-va... 1fe54b4 +/-
nemoclaw/src/se...cret-scanner.ts 100%
nemoclaw/src/commands/slash.ts 100%
nemoclaw/src/li...bprocess-env.ts 100%
nemoclaw/src/bl...eprint/state.ts 98%
nemoclaw/src/onboard/config.ts 98%
nemoclaw/src/bl...int/snapshot.ts 97%
nemoclaw/src/bl...print/runner.ts 95%
nemoclaw/src/co...ration-state.ts 94%
nemoclaw/src/bl...ate-networks.ts 94%
nemoclaw/src/index.ts 94%

TypeScript / code-coverage/cli

The overall coverage in the fix/vitest-hosted-va... branch is 47%. Coverage data for the main branch is not yet available.

Show a code coverage summary of the most covered files.
File main fix/vitest-hosted-va... 1fe54b4 +/-
src/lib/state/o...oard-session.ts 91%
src/lib/sandbox/config.ts 76%
src/lib/actions...dbox/rebuild.ts 73%
src/lib/onboard/preflight.ts 62%
src/lib/shields/index.ts 62%
src/lib/actions...licy-channel.ts 60%
src/lib/state/sandbox.ts 56%
src/lib/policy/index.ts 48%
src/lib/onboard...er-gpu-patch.ts 47%
src/lib/onboard.ts 19%

Updated June 27, 2026 04:44 UTC
Code Coverage is in Public Preview. Learn more and provide us with your feedback.

@github-actions

github-actions Bot commented Jun 27, 2026

Copy link
Copy Markdown
Contributor

E2E Advisor Recommendation

Required E2E: None
Optional E2E: hermes-e2e, cron-preflight-inference-local, diagnostics, upgrade-stale-sandbox

Dispatch hint: cron-preflight-inference-local-vitest,diagnostics-vitest,hermes-e2e-vitest,upgrade-stale-sandbox-vitest

Workflow run

Full advisor summary

E2E Recommendation Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required E2E

  • None. No required E2E is recommended because this PR is confined to E2E scenario tests, E2E workflow configuration, and E2E workflow-boundary tooling; it does not change NemoClaw runtime, installer, onboarding state machines, sandbox lifecycle implementation, credentials implementation, network policy assets, or assistant production code. The optional jobs above are useful to validate the modified CI lanes themselves.

Optional E2E

  • hermes-e2e (high): Validates the changed Hermes live scenario and workflow default model behavior across install.sh onboarding, Hermes sandbox health, and live hosted inference.
  • cron-preflight-inference-local (medium): Validates the changed cron preflight scenario now using the shared hosted inference default model for the inference.local managed-provider preflight path.
  • diagnostics (high): Validates the changed diagnostics scenario hosted inference config injection and secret redaction behavior around debug archives, install/onboard, sandbox diagnostics, and credential reset.
  • upgrade-stale-sandbox (high): Validates the changed stale sandbox upgrade helpers and hosted inference env propagation for install, upgrade-sandboxes --check, and rebuild of an old OpenClaw sandbox.

New E2E recommendations

  • None.

Dispatch hint

  • Workflow: .github/workflows/e2e-vitest-scenarios.yaml
  • jobs input: cron-preflight-inference-local-vitest,diagnostics-vitest,hermes-e2e-vitest,upgrade-stale-sandbox-vitest

@github-actions

github-actions Bot commented Jun 27, 2026

Copy link
Copy Markdown
Contributor

Vitest E2E Scenario Recommendation

Required Vitest E2E scenarios: cron-preflight-inference-local-vitest, diagnostics-vitest, upgrade-stale-sandbox-vitest
Optional Vitest E2E scenarios: None

Dispatch required Vitest E2E scenarios:

  • gh workflow run e2e-vitest-scenarios.yaml --ref <pr-head-ref> --field jobs=cron-preflight-inference-local-vitest
  • gh workflow run e2e-vitest-scenarios.yaml --ref <pr-head-ref> --field jobs=diagnostics-vitest
  • gh workflow run e2e-vitest-scenarios.yaml --ref <pr-head-ref> --field jobs=upgrade-stale-sandbox-vitest

Workflow run

Full Vitest E2E advisor summary

Vitest E2E Scenario Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required Vitest E2E scenarios

  • cron-preflight-inference-local-vitest: Focused free-standing Vitest job wired for changed live test test/e2e-scenario/live/cron-preflight-inference-local.test.ts.
    • Dispatch: gh workflow run e2e-vitest-scenarios.yaml --ref <pr-head-ref> --field jobs=cron-preflight-inference-local-vitest
  • diagnostics-vitest: Focused free-standing Vitest job wired for changed live test test/e2e-scenario/live/diagnostics.test.ts.
    • Dispatch: gh workflow run e2e-vitest-scenarios.yaml --ref <pr-head-ref> --field jobs=diagnostics-vitest
  • upgrade-stale-sandbox-vitest: Focused free-standing Vitest job wired for changed live test test/e2e-scenario/live/upgrade-stale-sandbox.test.ts.
    • Dispatch: gh workflow run e2e-vitest-scenarios.yaml --ref <pr-head-ref> --field jobs=upgrade-stale-sandbox-vitest

Optional Vitest E2E scenarios

  • None.

Relevant changed files

  • .github/workflows/e2e-vitest-scenarios.yaml
  • test/e2e-scenario/live/cron-preflight-inference-local.test.ts
  • test/e2e-scenario/live/diagnostics.test.ts
  • test/e2e-scenario/live/hermes-e2e.test.ts
  • test/e2e-scenario/live/upgrade-stale-sandbox-helpers.ts
  • test/e2e-scenario/live/upgrade-stale-sandbox.test.ts
  • tools/e2e-scenarios/workflow-boundary.mts

@github-actions

github-actions Bot commented Jun 27, 2026

Copy link
Copy Markdown
Contributor

PR Review Advisor — Changes requested

Merge posture: Do not merge yet
Primary next action: Resolve or justify PRA-1: Source-of-truth review needed: Hosted inference model selection in Hermes live Vitest Phase 5.
Open items: 0 required · 2 warnings · 0 suggestions · 8 test follow-ups
Since last review: 0 prior items resolved · 2 still apply · 0 new items found

Action checklist

  • PRA-1 Resolve or justify: Source-of-truth review needed: Hosted inference model selection in Hermes live Vitest Phase 5
  • PRA-2 Resolve or justify: Hermes chat payloads still bypass the hosted model source of truth in test/e2e-scenario/live/hermes-e2e.test.ts:104
  • PRA-T1 Add or justify test follow-up: Runtime validation
  • PRA-T2 Add or justify test follow-up: Runtime validation
  • PRA-T3 Add or justify test follow-up: Runtime validation
  • PRA-T4 Add or justify test follow-up: Runtime validation
  • PRA-T5 Add or justify test follow-up: Runtime validation
  • PRA-T6 Add or justify test follow-up: Acceptance clause
  • PRA-T7 Add or justify test follow-up: Acceptance clause
  • PRA-T8 Add or justify test follow-up: Acceptance clause

Findings index

ID Severity Category Location Required action
PRA-1 Resolve/justify architecture Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
PRA-2 Resolve/justify correctness test/e2e-scenario/live/hermes-e2e.test.ts:104 Make the Phase 5 request model come from the hosted config used for onboarding. For example, change chatPayload() to accept a model argument and call chatPayload(hosted.model, ...), or define the payload helper after const hosted = requireHostedInferenceConfig(...) and close over hosted.model.
Review findings by urgency: 0 required fixes, 2 items to resolve/justify, 0 in-scope improvements

⚠️ Resolve or justify before merge

Investigate these in the current review; either fix them, explain why they are not applicable, or document the accepted risk.

PRA-1 Resolve/justify — Source-of-truth review needed: Hosted inference model selection in Hermes live Vitest Phase 5

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as needs_followup.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: A compat-only env test should verify both Hermes direct-provider and sandbox inference.local chat payloads use requireHostedInferenceConfig(...).model.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: requireHostedInferenceConfig() honors NEMOCLAW_COMPAT_MODEL when NEMOCLAW_MODEL is absent; chatPayload() still serializes model: CHAT_MODEL, and CHAT_MODEL ignores NEMOCLAW_COMPAT_MODEL.

PRA-2 Resolve/justify — Hermes chat payloads still bypass the hosted model source of truth

  • Location: test/e2e-scenario/live/hermes-e2e.test.ts:104
  • Category: correctness
  • Problem: The PR now onboards Hermes with env from requireHostedInferenceConfig(), whose model precedence is NEMOCLAW_MODEL, then NEMOCLAW_COMPAT_MODEL, then the default. Phase 5 direct-provider and sandbox inference.local requests are still serialized by chatPayload() with the module-level CHAT_MODEL, which only reads NEMOCLAW_MODEL before falling back to DEFAULT_HOSTED_INFERENCE_MODEL. In a NEMOCLAW_COMPAT_MODEL-only lane, install/onboard can use one hosted-compatible model while the two chat probes validate another.
  • Impact: The E2E can give false confidence for the hosted-compatible credential/provider/model contract, and it does not fully satisfy the PR claim that affected tests use the shared hosted-compatible model/env plumbing consistently. It also weakens the security-test value of the live inference boundary because the sandbox inference.local route may not be exercised with the model actually staged by the hosted fixture.
  • Recommended action: Make the Phase 5 request model come from the hosted config used for onboarding. For example, change chatPayload() to accept a model argument and call chatPayload(hosted.model, ...), or define the payload helper after const hosted = requireHostedInferenceConfig(...) and close over hosted.model.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read test/e2e-scenario/fixtures/hosted-inference.ts and confirm requireHostedInferenceConfig() selects env.NEMOCLAW_COMPAT_MODEL when NEMOCLAW_MODEL is absent, then compare that with CHAT_MODEL and chatPayload() in test/e2e-scenario/live/hermes-e2e.test.ts.
  • Missing regression test: Add or identify coverage that sets only NEMOCLAW_COMPAT_MODEL and verifies both Hermes direct-provider and sandbox https://inference.local/v1/chat/completions request bodies use the same model returned by requireHostedInferenceConfig(...).model.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read test/e2e-scenario/fixtures/hosted-inference.ts and confirm requireHostedInferenceConfig() selects env.NEMOCLAW_COMPAT_MODEL when NEMOCLAW_MODEL is absent, then compare that with CHAT_MODEL and chatPayload() in test/e2e-scenario/live/hermes-e2e.test.ts.
  • Evidence: requireHostedInferenceConfig() computes model as env.NEMOCLAW_MODEL || env.NEMOCLAW_COMPAT_MODEL || options.model || DEFAULT_HOSTED_INFERENCE_MODEL, while hermes-e2e.test.ts defines CHAT_MODEL = process.env.NEMOCLAW_MODEL ?? DEFAULT_HOSTED_INFERENCE_MODEL and serializes model: CHAT_MODEL in chatPayload().

💡 In-scope improvements

These are lower-risk, not throwaway. Prefer fixing them in this PR when they are local to changed code; defer only with rationale or a linked follow-up.

  • None.
Simplification opportunities: 1 possible cut

These are safe simplification checks only. Do not remove validation, security controls, data-loss prevention, or required tests.

  • PRA-2 shrink (test/e2e-scenario/live/hermes-e2e.test.ts:104): The module-level CHAT_MODEL dependency for Phase 5 Hermes request payloads.
    • Replacement: Use the already-created hosted.model as the single source for direct-provider and sandbox inference.local chat payloads.
    • Net: 0 lines
    • Safety boundary: Keep trustedProviderEndpoint host allowlisting, API-key redactionValues, and the real provider/sandbox runtime boundaries intact.
Test follow-ups to resolve or justify

If these cover changed behavior, prefer adding them in this PR; otherwise state why existing coverage is enough or link the follow-up.

  • PRA-T1 Runtime validation — Hermes live E2E with only NEMOCLAW_COMPAT_MODEL set proves the direct-provider chat payload model equals requireHostedInferenceConfig(...).model.. The changed behavior depends on real installer/onboard flows, hosted inference, Docker/OpenShell, gateway credentials, and sandbox inference.local routing. Static review verifies the env plumbing but cannot prove the runtime boundary behavior.
  • PRA-T2 Runtime validation — Hermes live E2E with only NEMOCLAW_COMPAT_MODEL set proves the sandbox https://inference.local/v1/chat/completions payload model equals requireHostedInferenceConfig(...).model.. The changed behavior depends on real installer/onboard flows, hosted inference, Docker/OpenShell, gateway credentials, and sandbox inference.local routing. Static review verifies the env plumbing but cannot prove the runtime boundary behavior.
  • PRA-T3 Runtime validation — Diagnostics live E2E install receives COMPATIBLE_API_KEY, NEMOCLAW_PROVIDER=custom, the hosted endpoint, and the hosted model from hosted.env, and the generated debug archive excludes the exact API key and nvapi-shaped values.. The changed behavior depends on real installer/onboard flows, hosted inference, Docker/OpenShell, gateway credentials, and sandbox inference.local routing. Static review verifies the env plumbing but cannot prove the runtime boundary behavior.
  • PRA-T4 Runtime validation — Upgrade-stale live E2E install, upgrade-sandboxes --check, rebuild, and final check receive hosted.env and command artifacts redact hosted.apiKey.. The changed behavior depends on real installer/onboard flows, hosted inference, Docker/OpenShell, gateway credentials, and sandbox inference.local routing. Static review verifies the env plumbing but cannot prove the runtime boundary behavior.
  • PRA-T5 Runtime validation — Workflow-boundary validation covers hermes-e2e-vitest after removing the fixed minimax NEMOCLAW_MODEL override while still enforcing hosted-compatible inference mode when NVIDIA_INFERENCE_API_KEY is passed.. The changed behavior depends on real installer/onboard flows, hosted inference, Docker/OpenShell, gateway credentials, and sandbox inference.local routing. Static review verifies the env plumbing but cannot prove the runtime boundary behavior.
  • PRA-T6 Acceptance clause — Fixes hosted-compatible Vitest lanes that reached the custom provider path but still used public/provider-specific model or env setup, causing hosted endpoint validation to fail with HTTP 401/403. — add test evidence or identify existing coverage. Diagnostics and upgrade-stale now pass hosted.env from requireHostedInferenceConfig(), cron and Hermes default to DEFAULT_HOSTED_INFERENCE_MODEL, and the workflow removes the fixed Hermes minimax model. Hermes Phase 5 chat payloads still use CHAT_MODEL rather than hosted.model, so a NEMOCLAW_COMPAT_MODEL-only lane can still validate a different model from the hosted config.
  • PRA-T7 Acceptance clause — The affected tests now use the shared hosted-compatible model/env plumbing consistently. — add test evidence or identify existing coverage. Diagnostics, cron install/probe, and stale-upgrade command paths use requireHostedInferenceConfig()/hosted.env. Hermes install/onboard uses hosted.env, but direct-provider and inference.local chat payloads still use the local CHAT_MODEL source.
  • PRA-T8 Acceptance clause — Tests added or updated for changed behavior — add test evidence or identify existing coverage. The live Vitest files are updated, and support-tests/hosted-inference.test.ts covers hosted helper model precedence. There is no caller-specific coverage proving Hermes chatPayload() uses requireHostedInferenceConfig().model under NEMOCLAW_COMPAT_MODEL-only configuration.
Since last review details

Current findings, using the urgency labels above:

PRA-1 Resolve/justify — Source-of-truth review needed: Hosted inference model selection in Hermes live Vitest Phase 5

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as needs_followup.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: A compat-only env test should verify both Hermes direct-provider and sandbox inference.local chat payloads use requireHostedInferenceConfig(...).model.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: requireHostedInferenceConfig() honors NEMOCLAW_COMPAT_MODEL when NEMOCLAW_MODEL is absent; chatPayload() still serializes model: CHAT_MODEL, and CHAT_MODEL ignores NEMOCLAW_COMPAT_MODEL.

PRA-2 Resolve/justify — Hermes chat payloads still bypass the hosted model source of truth

  • Location: test/e2e-scenario/live/hermes-e2e.test.ts:104
  • Category: correctness
  • Problem: The PR now onboards Hermes with env from requireHostedInferenceConfig(), whose model precedence is NEMOCLAW_MODEL, then NEMOCLAW_COMPAT_MODEL, then the default. Phase 5 direct-provider and sandbox inference.local requests are still serialized by chatPayload() with the module-level CHAT_MODEL, which only reads NEMOCLAW_MODEL before falling back to DEFAULT_HOSTED_INFERENCE_MODEL. In a NEMOCLAW_COMPAT_MODEL-only lane, install/onboard can use one hosted-compatible model while the two chat probes validate another.
  • Impact: The E2E can give false confidence for the hosted-compatible credential/provider/model contract, and it does not fully satisfy the PR claim that affected tests use the shared hosted-compatible model/env plumbing consistently. It also weakens the security-test value of the live inference boundary because the sandbox inference.local route may not be exercised with the model actually staged by the hosted fixture.
  • Recommended action: Make the Phase 5 request model come from the hosted config used for onboarding. For example, change chatPayload() to accept a model argument and call chatPayload(hosted.model, ...), or define the payload helper after const hosted = requireHostedInferenceConfig(...) and close over hosted.model.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read test/e2e-scenario/fixtures/hosted-inference.ts and confirm requireHostedInferenceConfig() selects env.NEMOCLAW_COMPAT_MODEL when NEMOCLAW_MODEL is absent, then compare that with CHAT_MODEL and chatPayload() in test/e2e-scenario/live/hermes-e2e.test.ts.
  • Missing regression test: Add or identify coverage that sets only NEMOCLAW_COMPAT_MODEL and verifies both Hermes direct-provider and sandbox https://inference.local/v1/chat/completions request bodies use the same model returned by requireHostedInferenceConfig(...).model.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read test/e2e-scenario/fixtures/hosted-inference.ts and confirm requireHostedInferenceConfig() selects env.NEMOCLAW_COMPAT_MODEL when NEMOCLAW_MODEL is absent, then compare that with CHAT_MODEL and chatPayload() in test/e2e-scenario/live/hermes-e2e.test.ts.
  • Evidence: requireHostedInferenceConfig() computes model as env.NEMOCLAW_MODEL || env.NEMOCLAW_COMPAT_MODEL || options.model || DEFAULT_HOSTED_INFERENCE_MODEL, while hermes-e2e.test.ts defines CHAT_MODEL = process.env.NEMOCLAW_MODEL ?? DEFAULT_HOSTED_INFERENCE_MODEL and serializes model: CHAT_MODEL in chatPayload().

Workflow run details

This is an automated, non-binding review; it still expects maintainers and agents to respond to each required or warning item. Treat suggestions as current-PR improvements when they touch changed code; defer only with maintainer rationale or a linked follow-up. A human maintainer must make the final merge decision.

@github-actions

github-actions Bot commented Jun 27, 2026

Copy link
Copy Markdown
Contributor

PR Review Advisor (Nemotron Ultra) — No blocking findings

Merge posture: No blocking advisor findings
Primary next action: Add or justify PRA-T1 and any related test follow-ups.
Open items: 0 required · 0 warnings · 0 suggestions · 1 test follow-up
Since last review: 1 prior item resolved · 0 still apply · 0 new items found

Action checklist

  • PRA-T1 Add or justify test follow-up: Runtime validation
Test follow-ups to resolve or justify

If these cover changed behavior, prefer adding them in this PR; otherwise state why existing coverage is enough or link the follow-up.

  • PRA-T1 Runtime validation — Live E2E reruns in CI after merge will validate end-to-end hosted inference model/env behavior. Live E2E tests (gated by NEMOCLAW_RUN_E2E_SCENARIOS=1) are the intended validation layer for this integration change; unit tests for requireHostedInferenceConfig exist in test/fixtures/hosted-inference.test.ts; workflow validation runs in CI

Workflow run details

This is an automated, non-binding review; it still expects maintainers and agents to respond to each required or warning item. Treat suggestions as current-PR improvements when they touch changed code; defer only with maintainer rationale or a linked follow-up. A human maintainer must make the final merge decision.

Signed-off-by: Carlos Villela <cvillela@nvidia.com>
@cv

cv commented Jun 27, 2026

Copy link
Copy Markdown
Collaborator Author

Superseded by #5893 because force-push is disabled on PR branches and the replacement branch has a clean minimal diff.

@cv cv closed this Jun 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants