Skip to content

fix(inference): remove duplicate readiness comparison - #8437

Merged
cv merged 5 commits into
mainfrom
codex/fix-readiness-comparison-duplicate
Aug 6, 2026
Merged

fix(inference): remove duplicate readiness comparison#8437
cv merged 5 commits into
mainfrom
codex/fix-readiness-comparison-duplicate

Conversation

@cv

@cv cv commented Aug 6, 2026

Copy link
Copy Markdown
Collaborator

Summary

The main branch fails CLI type checking because it contains two private readinessComparisonMatches functions after changes from #8399 and #8391 were combined.
This change removes the superseded implementation and retains the dotted numeric version comparison covered by the resolver tests.
It also aligns the positive storage-remediation test data with the host-local preset that the test resolves.

Changes

  • Remove the earlier versionAtLeast helper and duplicate readinessComparisonMatches function.
  • Retain compareNumericDottedVersions and the single resolver call path added by fix(inference): honor host-local serving recipe contracts #8391.
  • Add a resolver regression for a version segment above Number.MAX_SAFE_INTEGER.
  • Build the positive storage-remediation test data from the host-local preset under test.

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:
  • Tests not applicable — justification:
  • Docs updated for user-facing behavior changes
  • Docs not applicable — justification: This removes a duplicate private implementation and preserves the resolver contract, schema, call site, and user-visible behavior.
  • 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: Independent Codex Desktop review passed all nine security categories for PR commit 05999730c; the deletion retains the stricter validation path, and the test-fixture change does not alter production behavior.
  • Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue:

Documentation Writer Review

  • Documentation writer subagent reviewed the completed changes
  • Result: no-docs-needed
  • Evidence: PR commit 05999730c contains the duplicate-removal repair, resolver regression, and test-fixture alignment without changing a user-visible API, CLI, configuration, workflow, default, error, or supported behavior.
  • Agent: Codex Desktop

DGX Station Hardware Evidence

  • Tested on DGX Station
  • Tested commit: Not applicable.
  • Station profile/scenario: Not applicable.
  • Result: Not applicable.
  • Supporting evidence: Not applicable.

Verification

  • PR description includes a Signed-off-by: line and every commit appears as Verified in GitHub
  • Normal pre-commit, commit-msg, and pre-push hooks passed, or npm run validate:pr passed after refreshing origin/main when hooks were skipped or unavailable
  • Targeted behavior tests pass for the current change set, or tests are marked not applicable above — command/result or justification: GitHub Actions is running for PR commit 05999730c; no standalone local test suite ran.
  • Applicable broad gate passed — npm test for broad runtime/test-harness changes; npm run check for repo-wide validation/coverage changes — command/result: Required GitHub Actions checks are pending; no standalone local test suite ran.
  • 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)

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

Signed-off-by: Carlos Villela <cvillela@nvidia.com>
@cv cv added the area: inference Inference routing, serving, model selection, or outputs label Aug 6, 2026
@cv cv self-assigned this Aug 6, 2026
@coderabbitai

coderabbitai Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

The resolver now validates numeric dotted versions with safe integers and rejects invalid inputs. It also evaluates scoped readiness requirements across qualifications, observations, capabilities, and comparisons, including remediable storage handling.

Changes

Readiness Resolver

Layer / File(s) Summary
Numeric version comparison
src/lib/inference/serving/resolver.ts, src/lib/inference/serving/resolver.test.ts
The resolver uses safe numeric dotted-version parsing for version-at-least checks. The test rejects a version with a segment above Number.MAX_SAFE_INTEGER.
Readiness requirement matching
src/lib/inference/serving/resolver.ts, src/lib/inference/serving/resolver.test.ts
The resolver matches scoped qualifications, observations, capabilities, and comparison-based observations. It handles the remediable storage capability case. Tests pass the selected preset to storage-remediation reports.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

  • NVIDIA/NemoClaw#8391: Both PRs update readiness comparison logic and numeric dotted-version validation.
  • NVIDIA/NemoClaw#8402: Both PRs modify readiness handling and tests for remediable Docker storage conflicts.

Suggested labels: platform: dgx-spark

Suggested reviewers: prekshivyas, ericksoa

🚥 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
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.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the main change: removing the duplicate readiness comparison from inference serving.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/fix-readiness-comparison-duplicate

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

@github-code-quality

github-code-quality Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Code Coverage Overview

Languages: TypeScript

TypeScript / code-coverage/plugin

The overall coverage in commit 0599973 in the codex/fix-readiness-... branch remains at 96%, unchanged from commit 7926166 in the main branch.

TypeScript / code-coverage/cli

The overall coverage in commit 0599973 in the codex/fix-readiness-... branch remains at 82%, unchanged from commit bf429e9 in the main branch.

Show a code coverage summary of the most impacted files.
File main bf429e9 codex/fix-readiness-... 0599973 +/-
src/lib/domain/.../connect-env.ts 97% 89% -8%
src/lib/inferen...al-lifecycle.ts 89% 86% -3%
src/lib/messagi...tatus-health.ts 92% 89% -3%
src/lib/onboard/command.ts 97% 95% -2%
src/lib/shields/index.ts 81% 81% 0%
src/lib/state/o...oard-session.ts 89% 89% 0%
src/lib/inference/vllm.ts 87% 88% +1%
src/lib/actions...licy-channel.ts 81% 83% +2%
src/lib/inferen...profile-list.ts 0% 86% +86%
src/lib/inferen...e-provenance.ts 0% 86% +86%

Updated August 06, 2026 06:00 UTC

@github-actions

github-actions Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

PR Review Advisor — No blocking findings reported

Advisor assessment: No blocking advisor findings reported
Next action: No advisor follow-up needed.
Findings: 0 blockers · 0 warnings · 0 suggestions

Model lanes

  • GPT-5.6 Terra (primary): Completed · high confidence · 0 blockers · 0 warnings · 0 suggestions
  • Nemotron 3 Ultra (second opinion): Completed · high confidence · 0 blockers · 0 warnings · 0 suggestions
  • Model comparison: normalized findings match; normalized terminology decisions differ; normalized E2E selections differ; severity counts match.
1 terminology difference from the second opinion

Advisory only. These are normalized differences from the primary terminology receipt.

  • version segment at src/lib/inference/serving/resolver.test.ts:633: selected only by the second-opinion lane as justified.
1 additional E2E selection from the second opinion

Advisory only. The primary lane did not select these E2E jobs or targets.

  • gpu-e2e: The completed second-opinion lane identified E2E coverage that the primary lane omitted.

Second-opinion terminology and E2E selections are advisory. They do not change the primary assessment or E2E / PR Gate.

E2E guidance

Advisory only. E2E / PR Gate selects and runs jobs independently.

Recommended E2E: inference-routing, network-policy

1 optional E2E recommendation
  • vllm-docker-storage

Workflow run details

This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge.

Signed-off-by: Carlos Villela <cvillela@nvidia.com>
cv added 3 commits August 5, 2026 22:22
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
@cv
cv merged commit e40ca39 into main Aug 6, 2026
95 of 98 checks passed
@cv
cv deleted the codex/fix-readiness-comparison-duplicate branch August 6, 2026 06:11
@github-actions github-actions Bot added the v0.0.104 Release target label Aug 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: inference Inference routing, serving, model selection, or outputs v0.0.104 Release target

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant