fix(inference): raise Gemini health probe budget (Fixes #10260) - #10311
fix(inference): raise Gemini health probe budget (Fixes #10260)#10311deepujain wants to merge 10 commits into
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (16)
🚧 Files skipped from review as they are similar to previous changes (9)
Included review availability: Your plan provides up to 12 included reviews per hour; 8 remain after this review. 📝 WalkthroughWalkthroughInference probes now use provider-specific reply-token budgets, including 256 tokens for Gemini. Sandbox request and readiness timeouts increase to 90 and 95 seconds. Health and onboarding tests verify payloads, retries, response validation, and documentation consistency. ChangesInference probe budgets
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to Gemini inference probes now receive a larger reply budget while malformed or empty responses remain rejected. No current merge-blocking risk is identified. Sequence Diagram(s)sequenceDiagram
participant Onboarding
participant BudgetResolver
participant ProbePayload
participant GeminiAPI
Onboarding->>BudgetResolver: resolve provider and reply budget
BudgetResolver-->>Onboarding: return 256 for gemini-api
Onboarding->>ProbePayload: build Chat Completions request
ProbePayload->>GeminiAPI: send max_tokens: 256
GeminiAPI-->>Onboarding: return probe response
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Out of Scope Changes checkExplanation Most changes support provider-specific Gemini probe budgets, but the PR also increases the readiness timeout from 30 seconds to 95 seconds and introduces a 90-second request timeout. The linked issue requires a higher Gemini output budget and states that timeout validation remains unchanged. These timeout changes are not supported by the stated issue objective. Full details: Docstring CoverageExplanation Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 20 functions across 16 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
ce2df2e to
4e8c3d3
Compare
cjagwani
left a comment
cjagwani
left a comment
There was a problem hiding this comment.
Reviewed exact head 4e8c3d3.
Approved. The accepted QA issue #10260 establishes scope, and the implementation is the smallest provider-specific correction: Gemini chat-completions health probes receive a 256-token output ceiling, while every non-Gemini path retains the 16-token probe. Host and in-sandbox probes agree, and malformed, empty, error-envelope, credential, timeout, endpoint, and response-size behavior remains fail-closed.
Security review:
- Secrets and credentials: PASS — credentials remain in the existing trusted auth-config or OpenShell route boundary and are not added to argv or diagnostics.
- Input validation and sanitization: PASS — response bodies still require a structurally valid Chat Completions result; null-only Gemini content remains unhealthy.
- Authentication and authorization: PASS — existing Bearer/auth-config and recorded-gateway authority are unchanged.
- Dependencies: PASS — no dependency changes.
- Error handling and logging: PASS — response and credential values are not surfaced in returned diagnostics.
- Cryptography and data protection: PASS — not applicable; no cryptographic operations changed.
- Configuration and security headers: PASS — endpoint, headers, timeout, and response-size limits are unchanged.
- Security testing: PASS — the exact focused suite passed 90/90 and covers the Gemini payload plus strict invalid-response rejection.
- Holistic security posture: PASS — no SSRF, network-policy, sandbox, privilege, or credential-custody surface is widened.
Files reviewed: src/lib/actions/sandbox/inference-invocation-probe.ts and test, src/lib/inference/health.ts and test, and src/lib/inference/max-tokens-field.ts. All three commits are GitHub Verified. No actionable findings.
This approval does not waive merge gates: NVIDIA external-contributor vetting still must authorize the normal workflows, required CI must pass, and the failed PR Review Advisor infrastructure run should be rerun or explicitly handled by a maintainer.
4e8c3d3 to
553c51e
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
553c51e to
71d7167
Compare
wscurran
added
area: inference
71d7167 to
d9875ed
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
d3800aa to
4a77d52
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
4a77d52 to
cceae79
Compare
cjagwani
left a comment
cjagwani
left a comment
There was a problem hiding this comment.
[P1 gate blocker] Refresh this branch on current main before merge.
Exact-head rereview of cceae798e55728552c1eeac1448c3410a52ec8d7 found the five-file patch byte-for-byte identical to the previously approved 4a77d521cdebb4ac918939d24d67b0f29611df53; no code or security regression was introduced. npm run build:cli, npm run typecheck:cli, diff hygiene, and 110 focused CLI assertions pass. All nine exact-head Advisor artifacts report no finding, CodeRabbit has no current actionable thread, both replacement commits are GitHub Verified, and both carry Signed-off-by declarations.
The current head was rebuilt on 1f90124e0f416139cbcc9745b6c236adc62f4433, but main advanced to 8708e19b1ff01590ab1147341dadb5f0c7b921e8 when #10515 merged before this CI run. The governance check intentionally evaluates BASE_SHA..HEAD_SHA, so this exact head deterministically fails three growth assertions on base-only files outside the five-file PR diff. This is not an infrastructure-only rerun candidate.
Impact: the current head is blocked and cannot satisfy the green-gate requirement. Smallest safe fix: rebase the unchanged five-file patch onto current main, preserve signed and Verified commits, and let normal DCO, CodeQL, required CI, and governance checks rerun. Regression evidence: the growth guard and every required gate must be green on the resulting exact head.
Nine-category security verdict: PASS secrets/credentials, input validation/sanitization, authentication/authorization, dependencies, error handling/logging, cryptography/data protection, configuration/security headers, security testing, and holistic security posture. No source-line blocker was found; this request is solely for the deterministic current-base gate failure.
353b97d to
87bfed5
Compare
cjagwani
left a comment
cjagwani
left a comment
There was a problem hiding this comment.
Reviewed exact head 87bfed5c7c55bf6f7a4f13cf7c1790392c8bce8b.
The accepted QA issue #10260 establishes scope, and the runtime behavior remains narrow: Gemini gets a 256-token probe budget while strict response validation, credential custody, fixed endpoints, bounded output, and non-Gemini budgets remain intact. The detached exact-head build and CLI typecheck pass; 91 focused CLI assertions pass; diff hygiene is clean. Both rewritten commits are GitHub Verified and carry Signed-off-by declarations. CodeRabbit has no current actionable thread.
REQUEST_CHANGES is for the three exact-line current-head Advisor findings: centralize the provider-to-budget policy, pass the override through the existing Chat Completions payload owner instead of reparsing generated curl argv, and correct the inaccurate budget rationale. Smallest fixes and required regressions are in the inline comments. I did not authorize NVIDIA runner workflows while code blockers remain.
Nine-category security verdict:
- Secrets/credentials: PASS — no credential is added to argv, payload logs, or sandbox commands.
- Input validation/sanitization: PASS — malformed, error-envelope, and empty Gemini replies remain unhealthy.
- Authentication/authorization: PASS — existing trusted auth-config and OpenShell route authority are unchanged.
- Dependencies/third-party: WARNING — no package change, but the new argv reparse duplicates the existing payload-builder capability.
- Error handling/logging: PASS — bounded response handling and redacted fixed diagnostics remain intact.
- Cryptography/data protection: PASS/N/A — no cryptographic operation changes.
- Configuration/security headers: PASS — endpoints, auth headers, response limit, and network policy are unchanged.
- Security testing: PASS — exact-head focused coverage is green; requested builder/resolver tests protect the remaining ownership fixes.
- Holistic posture: WARNING — duplicated provider-budget policy can let host and sandbox health behavior drift and recreate the false-unhealthy failure.
9f9fb16 to
4c38d00
Compare
cjagwani
left a comment
cjagwani
left a comment
There was a problem hiding this comment.
Reviewing exact head 4c38d00. The three prior ownership/rationale findings are resolved, but two current blockers remain: the Gemini reply budget still does not reach the actual onboarding or optimized-validation smoke paths, and the sandbox regression inspects generated shell text instead of executing the request boundary.
Scope is accepted by #10260. All nine PR Review Advisor artifacts completed: Behavior, Architecture, and Operations converge on the onboarding gap; Test Design flags the source-shape regression; the other specialists found no additional blocker. CodeRabbit has no actionable current-head feedback. All three commits are GitHub Verified and include Signed-off-by declarations. Normal required CI, CodeQL, and DCO workflows are absent/skipped behind external-contributor validation, so this head cannot be approved yet.
Exact-head verification in an isolated detached worktree: npm ci prepare/build passed; CLI type-check passed; focused tests passed 166 with 1 skipped; source-shape scan and diff hygiene passed.
Security categories: secrets PASS; input validation PASS; authentication/authorization PASS; dependencies PASS; error handling/logging WARNING (valid Gemini onboarding can still abort false-unhealthy); cryptography/data protection PASS; configuration PASS; security testing FAIL (missing executed onboarding/sandbox boundary regression); holistic review FAIL until the split onboarding budget authority is removed.
cjagwani
left a comment
cjagwani
left a comment
There was a problem hiding this comment.
Reviewed exact head 69ac357ae6538b0c664a7d91ca2f0981dad9ff35 against current base 6831ed251641b6885939deef6dd34bf3e10266b0.
The rebase is patch-for-patch identical to the previously reviewed series. The prior source blockers remain resolved. Local exact-head validation passed 278 focused assertions (one platform skip); all 10 commits are Verified and signed off; CodeRabbit is clean; the nine-category source security review passed.
[P1] Required exact-head CI cannot run for this fork (maintainer-owned). The openshell-sdk-package job fails with “The reviewed OpenShell SDK is available only to same-repository pull requests,” and cli-tests then fails before its shards run. Impact: required CI cannot produce approval evidence for this head, even though the patch is locally clean. Smallest safe next action: a NemoClaw maintainer should port this already-reviewed, patch-equivalent change to a same-repository branch/PR with signed and GitHub-Verified history, or provide an approved trusted fork-compatible SDK packaging path. Regression proof: the OpenShell SDK package gate, CLI tests, DCO, CodeQL, and the required PR/E2E gates must all pass on that exact head and base.
This blocker is in the repository workflow boundary, not a changed source line, so there is no honest inline location to attach it to.
| payload: { | ||
| model: input.model, | ||
| [resolveMaxTokensField(input.model)]: MIN_PROBE_REPLY_TOKENS, | ||
| [resolveMaxTokensField(input.model)]: resolveProbeReplyTokens(input.provider), |
There was a problem hiding this comment.
[P1] Add runtime proof for the changed Gemini route
Impact: this changes the real Hermes status/start request through inference.local, but current tests replace curl or inject a successful executor. They can pass while OpenShell rejects the 256-token payload or the 95-second wrapper cancels the real request; the exact boundary from #10260 remains unproved.
Smallest safe fix: add or extend a credential-backed live E2E lane that onboards Hermes with gemini-api / gemini-2.5-flash and invokes normal status or start. Regression proof: observe a healthy result or zero exit through the actual inference.local route, fail with the former 16-token budget, and fail if the wrapper ends before the 90-second request deadline.
There was a problem hiding this comment.
The requested credential-backed proof cannot be produced independently on this fork: the repository has no Gemini live E2E lane or GEMINI_API_KEY workflow wiring, and external-contributor workflows cannot access maintainer secrets. The exact request boundary is exercised with a controlled curl consumer, including the 256-token payload and 90/95-second deadline relationship. I am leaving this thread open for maintainer-owned credentialed validation rather than adding a lane that cannot execute.
There was a problem hiding this comment.
Confirmed as an external validation blocker on current head 65a9ca8. The repository has no credential-backed Gemini live E2E lane or GEMINI_API_KEY workflow wiring to extend. I will not fabricate runtime evidence or add an unexecutable secret-dependent lane. The exact head passes validate:pr and exercises the shell-to-curl boundary locally; maintainer infrastructure is still required to prove the real inference.local route with Gemini credentials.
69ac357 to
d700948
Compare
cjagwani
left a comment
cjagwani
left a comment
There was a problem hiding this comment.
Reviewed exact head d700948e5070688f878df28d63d4bbe56d33be5f against live base 3d754417b5f07f9ed61ff709c5bb107aaffff1dc.
The 10-commit patch series is patch-for-patch identical to the reviewed 69ac357a series. Local exact-head validation again passes 278 focused assertions with one platform skip; diff hygiene is clean; all commits are signed off and GitHub Verified; the source/security verdict remains PASS.
[P1] This refresh is already behind the live base. Its merge base is 6831ed25, not 3d754417. The trusted growth guard therefore compares base-only E2E changes as regressions and deterministically fails. Impact: this head cannot satisfy the current-base governance gate. Smallest safe fix: rebase the unchanged patch onto current main and preserve signed, Verified history. Regression proof: the growth guard must pass on the resulting exact head/base.
The existing P1 runtime-evidence blocker also remains: the real Hermes → inference.local → Gemini status/start path is still not covered; the current tests use fake curl or injected executors. Add or extend credential-backed live E2E coverage that proves the normal route with gemini-api / gemini-2.5-flash.
Finally, normal CI still cannot validate this fork because the trusted OpenShell SDK package is restricted to same-repository PRs. After the source/test blockers are resolved, a maintainer must port the patch to a compliant same-repository branch/PR, or approve a trusted fork-compatible packaging path, and obtain all required green gates on that exact head.
|
Addressed the exact-head merge-gate blocker by refreshing the unchanged patch onto current main at |
|
Thanks—the branch is not on current Please refresh again after verifying the live base OID immediately before the push. That resolves only the current-base blocker; the Gemini live runtime-evidence requirement and the fork-only OpenShell SDK CI boundary remain. |
d700948 to
65a9ca8
Compare
| payload: { | ||
| model: input.model, | ||
| [resolveMaxTokensField(input.model)]: MIN_PROBE_REPLY_TOKENS, | ||
| [resolveMaxTokensField(input.model)]: resolveProbeReplyTokens(input.provider), |
There was a problem hiding this comment.
[P1] The current-base refresh fixes the growth gate, but this runtime proof is still missing.
Impact: tests replace curl or inject a successful executor, so they can pass while the real Hermes status/start request fails across OpenShell and inference.local—the exact boundary reported by #10260.
Smallest safe fix: add or extend credential-backed live E2E coverage for gemini-api / gemini-2.5-flash through the normal status or start path. Regression proof: observe healthy or zero exit through the actual inference.local route, fail with the former 16-token budget, and fail if the wrapper ends before the 90-second request deadline.
cjagwani
left a comment
cjagwani
left a comment
There was a problem hiding this comment.
Reviewed exact head 65a9ca82847a17a268506e74f99cc3c596744992 against current base 3d754417b5f07f9ed61ff709c5bb107aaffff1dc.
The current-base blocker is fixed and the codebase growth guard now passes. The refreshed series is behavior-equivalent to the previously reviewed patch. Local exact-head validation passes 278 focused assertions with one platform skip; diff hygiene is clean; all 10 commits are signed off and GitHub Verified; CodeRabbit is clean; and the nine-category source security review remains PASS.
[P1] The reported production boundary still lacks regression proof. The current tests replace curl, use a local fake HTTP server, or inject a successful executor. They therefore do not prove the real Hermes → normal start/status → OpenShell → https://inference.local → Gemini route from #10260. Impact: the suite can pass while the original Ready-sandbox failure still occurs. Smallest safe fix: add or extend credential-backed live E2E coverage for gemini-api / gemini-2.5-flash through the normal status or start path. Regression proof: observe a healthy/zero result through the actual route, fail with the former 16-token budget, and fail if the wrapper ends before the 90-second request deadline. The exact-line comment identifies the relevant runtime call site.
The same-repository CI boundary also remains maintainer-owned: this fork cannot obtain the trusted OpenShell SDK package. Once the runtime coverage is present, a maintainer must port the reviewed patch to a compliant same-repository branch/PR or approve a trusted fork-compatible packaging path, then obtain all required green gates on that exact head. The current-head PR Review Advisor run is still in progress.
65a9ca8 to
e0b5ca5
Compare
|
Rebased the unchanged patch onto current main at |
cjagwani
left a comment
cjagwani
left a comment
There was a problem hiding this comment.
Reviewed exact head e0b5ca5 against live base 2afbb2f.
The refreshed patch is behavior-equivalent to the previously reviewed series. Local exact-head verification passed the CLI and plugin builds, CLI typecheck, repository structural checks, strict docs build, diff hygiene, and 278 focused assertions with one platform skip. All 10 commits are signed off and GitHub Verified. CodeRabbit has no actionable current-head finding. After one infrastructure-only retry of confirmed provider-rate-limit failures, all nine PR Review Advisor specialists completed; no specialist found a current blocking source defect.
[P1] The reported production boundary still lacks exact-head live regression proof. The checked-in tests replace curl, use a local HTTP server, or inject an executor, so they do not prove the real Hermes start/status through OpenShell and https://inference.local to Gemini from #10260. The existing current exact-line thread remains the canonical blocker: #10311 (comment). Impact: the suite can pass while the original Ready-sandbox false-unhealthy failure remains. Smallest safe next action: an authorized maintainer must run credential-backed live validation for gemini-api / gemini-2.5-flash through the normal lifecycle path. Regression proof: healthy status through the actual route on this exact head, with the former 16-token budget and an outer deadline shorter than the 90-second request both demonstrated to fail.
[P2] Six required checks are absent on this fork: checks, check-hash, changes, commit-lint, dco-check, and E2E / PR Gate; package-openshell-sdk is skipped. An authorized maintainer must permit the normal external-contributor workflows or port the reviewed patch to a compliant same-repository branch, then obtain every required green gate on the same exact head.
Nine-category security verdict: PASS for secrets and credentials, input validation, authentication and authorization, dependencies, error handling and logging, cryptography and data protection, configuration, security testing of the checked-in boundaries, and holistic posture. The Advisor architecture note about future provider-extension ownership is nonblocking because it identifies no current contract failure. No contributor branch was modified.
6e6e90b to
530375e
Compare
|
Rebased the unchanged patch onto current main d6e8543; exact head is 530375e. npm run validate:pr passes after rebuilding the nested workspace output. Current Advisor failures exhausted five provider retries with HTTP 429, no body, and zero review output; NVIDIA workflow approvals remain external. The credential-backed Gemini runtime proof remains blocked on a maintainer-provided secret-backed lane. |
|
Latest PR commit 530375e now has all nine Advisor specialists, CodeRabbit, and source guardrails passing. The nested plugin build and npm run validate:pr also pass against main d6e8543. Credential-backed Gemini proof through the real inference.local route remains blocked on a maintainer-provided secret-backed lane, and fork workflows require NVIDIA vetter approval. |
|
Resolved the commands.mdx rebase conflict by preserving current main retry documentation and the PR start-probe budget contract. Latest PR commit 54b686f passes npm run build:cli, CLI typecheck, 42 focused tests, the nested nemoclaw build, and npm run validate:pr. Eight Advisor specialists passed; Verification evidence timed out after 900 seconds with zero output, and credential-backed Gemini runtime proof still requires maintainer infrastructure. |
Fixes NVIDIA#10260 Signed-off-by: Deepak Jain <deepujain@gmail.com>
Fixes NVIDIA#10260 Signed-off-by: Deepak Jain <deepujain@gmail.com>
Fixes NVIDIA#10260 Signed-off-by: Deepak Jain <deepujain@gmail.com>
Fixes NVIDIA#10260 Signed-off-by: Deepak Jain <deepujain@gmail.com>
Fixes NVIDIA#10260 Signed-off-by: Deepak Jain <deepujain@gmail.com>
Signed-off-by: Deepak Jain <deepujain@gmail.com>
Derive the Gemini reply budget inside probeOpenAiLikeEndpointOptimized so onboarding callers only pass provider identity. Replace the mocked smoke regression with real optimized HTTP payload assertions for Gemini 256, ordinary 16, and DeepSeek 8192 budgets. Fixes NVIDIA#10260 Signed-off-by: Deepak Jain <deepujain@gmail.com>
Centralize onboarding reply-budget resolution and pass the resolved budget through doubled-timeout chat-completions retries. Fixes NVIDIA#10260 Signed-off-by: Deepak Jain <deepujain@gmail.com>
Keep the architecture budget aligned with the reduced dependency count. Signed-off-by: Deepak Jain <deepujain@gmail.com>
Restore direct policy imports and document provider-specific budgets so runtime behavior and operator guidance stay aligned. Signed-off-by: Deepak Jain <deepujain@gmail.com>
|
PR Review Advisor finished for commit |
3 participants
Summary
Gives Gemini probes enough output budget for reasoning before acknowledgement while preserving ordinary and model-specific budgets across health, onboarding, retry, and sandbox readiness paths.
Fixes #10260
Changes
Verification
npm run build:cli, the nestednemoclawbuild, andnpm run validate:prpassed for commit503b9bf2aagainstmainfa08360a2.main.Remaining external blocker
Maintainer review requests credential-backed live proof through the real Hermes
inference.localroute. The repository currently has no Gemini live E2E lane orGEMINI_API_KEYworkflow wiring to extend. Maintainer infrastructure and credentials are required for that proof.Signed-off-by: Deepak Jain deepujain@gmail.com