fix(onboard): honor extended validation timeout budgets - #4449
Conversation
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughDerives probe process timeouts from curl ChangesCurl Timeout Resolution and Validation Budget Configuration
🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Comment |
E2E Advisor RecommendationRequired E2E: Dispatch hint: Auto-dispatched E2E: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
E2E Scenario Advisor RecommendationRequired scenario E2E: None Full scenario advisor summaryE2E Scenario AdvisorBase: Required scenario E2E
Optional scenario E2E
Relevant changed files
|
PR Review AdvisorFindings: 0 needs attention, 6 worth checking, 1 nice ideas Review findings🛠️ Needs attention
🔎 Worth checking
🌱 Nice ideas
Since last review detailsCurrent findings:
This is an automated advisory review. A human maintainer must make the final merge decision. |
Selective E2E Results — ✅ All requested jobs passedRun: 26593178159
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/lib/adapters/http/probe.test.ts`:
- Around line 94-116: getCurlMaxTimeSeconds currently picks the first --max-time
occurrence which contradicts curl's behavior; update getCurlMaxTimeSeconds (in
src/lib/adapters/http/probe.ts) to parse the args and use the last repeated
--max-time value (falling back to default when absent) so the wrapper timeout
calculation is correct, then add a regression test in probe.test.ts that calls
runCurlProbe with duplicated flags (e.g., "--max-time", "15", "--max-time",
"120") and asserts the computed timeout uses the last value (expecting 125_000
ms when wrapper adds the extra buffer).
In `@src/lib/adapters/http/probe.ts`:
- Around line 75-87: getCurlMaxTimeSeconds currently returns on the first match,
but curl uses last-set-wins; update getCurlMaxTimeSeconds to scan all argv
entries and keep the last valid parsed value (handling both "--max-time"
followed by a value and "--max-time=<value>") instead of returning immediately,
then return that last valid number or null if none found; ensure you still
validate Number.isFinite(value) && value > 0 when storing the last seen value.
🪄 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: f5d3bb19-9557-4082-bdb0-3cb8e5314d54
📒 Files selected for processing (4)
src/lib/adapters/http/probe.test.tssrc/lib/adapters/http/probe.tssrc/lib/inference/onboard-probes.test.tssrc/lib/inference/onboard-probes.ts
Selective E2E Results — ✅ All requested jobs passedRun: 26593343487
|
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
|
Actionable comments posted: 0 |
Selective E2E Results — ✅ All requested jobs passedRun: 26596877131
|
Summary
Increase NemoClaw onboarding validation tolerance for slow NVIDIA Endpoints model probes without broadening the default 15s probe path for every model. This targets the observed timeout when validating
qwen/qwen3.5-397b-a17banddeepseek-ai/deepseek-v4-flashfrom build.nvidia.com.Related Issue
None filed yet. Thread context: NVIDIA Endpoints chat-completions probe times out at 15s during
nemoclaw onboard, while the same request succeeds withcurl --max-time 300.Changes
--max-timeso extended curl budgets are not killed early by Node's wrapper timeout.NEMOCLAW_ONBOARD_VALIDATION_TIMEOUT_SECONDSfloor for non-interactive/on-call validation override.Release Target / Blast Radius
Targeting
v0.0.55, pending blast-radius review. This is intentionally a draft PR until we confirm the widened validation path is acceptable for onboard UX and CI runtime.Expected blast radius is limited to onboarding-time inference validation and shared curl probe process timeouts. Runtime inference routing is unchanged; model payloads are unchanged except for existing model-specific paths.
Type of Change
Verification
npx prek run --all-filespassesnpm testpassesnpm run docsbuilds without warnings (doc changes only)Focused verification run:
npm run build:clinpm run typecheck:clinpx vitest run src/lib/adapters/http/probe.test.ts src/lib/inference/onboard-probes.test.ts test/wsl2-probe-timeout.test.tsnpx vitest run test/onboard-selection.test.ts -t "NVIDIA Endpoints"git diff --checkSigned-off-by: Aaron Erickson aerickson@nvidia.com
Summary by CodeRabbit
New Features
Improvements
Tests