fix(onboard): bound Responses API streaming probe timeout - #7884
Merged
Conversation
Onboarding validates a custom OpenAI-compatible endpoint by probing /responses in streaming mode to confirm it emits response.output_text.delta. An endpoint that returns 200 and opens the SSE stream but never emits a delta (a stalled or lifecycle-only stream) made the probe wait the full validation budget — 15s standard, up to 60s calibrated — hanging the inference step long enough that an onboarding harness times out (EXPECT_TIMEOUT / exit 1). The streaming probe only needs the first delta to decide and already falls back to /chat/completions on failure, so give it a dedicated short deadline: STREAMING_EVENT_PROBE_MAX_SECONDS = 12s, applied as min() with the current budget (never lengthens) on both the curl and validation-session paths. Verified on real hardware against the same host/endpoint: the streaming stall drops ~27s -> ~12s and the onboard inference step ~40s -> ~22s. Signed-off-by: Hung Le <hple@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. |
Contributor
|
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:
📝 WalkthroughWalkthroughStreaming Responses probes now use a dedicated 12-second timeout cap. Onboarding uses streaming-specific curl arguments, and the validation session applies the same bound to native streaming requests. ChangesStreaming probe timeout handling
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
1 additional E2E selection from the second opinionAdvisory only. The primary lane did not select these E2E jobs or targets.
Second-opinion E2E selections are advisory. They do not change the primary assessment or E2E / PR Gate. E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: 1 warning · 0 suggestionsWarningsWarnings do not block.
|
Contributor
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit 86b5c38 in the TypeScript / code-coverage/cliThe overall coverage in commit 86b5c38 in the Show a code coverage summary of the most impacted files.
Updated |
hunglp6d
marked this pull request as ready for review
July 30, 2026 07:05
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 `@src/lib/inference/probe-http-helpers.test.ts`:
- Around line 79-95: The new streaming event probe tests lack the required
issue-reference suffix in their titles. Update each test title in the affected
test block, including “caps the streaming event probe max-time below the
standard budget” and “keeps the streaming event probe bounded even when the env
raises the budget,” to end with “(`#7792`)” while preserving their
behavior-oriented wording.
🪄 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: b75ae040-cd3d-46d0-8320-83a4ad9313df
📒 Files selected for processing (4)
src/lib/inference/onboard-probes.tssrc/lib/inference/openai-validation-session.tssrc/lib/inference/probe-http-helpers.test.tssrc/lib/inference/probe-http-helpers.ts
Signed-off-by: Hung Le <hple@nvidia.com>
hunglp6d
added
area: onboarding
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/lib/inference/openai-validation-session.test.ts (1)
162-175: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRestore the timer spy on every exit path.
Line 175 runs only when
probeOpenAiLikeEndpointWithValidationSessionresolves. If it rejects, the test skipsmockRestore()and leaves the global spy active until suite cleanup. Move the probe, call inspection, and assertions intotry, and restore the spy infinally.As per coding guidelines, deterministic tests must restore spies. Based on learnings, locally created spies should be restored explicitly on failure paths.
🤖 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 `@src/lib/inference/openai-validation-session.test.ts` around lines 162 - 175, Update the test around probeOpenAiLikeEndpointWithValidationSession so timeoutSpy is always restored: place the probe call, timeout-call inspection, and related assertions in a try block, and move timeoutSpy.mockRestore() into finally. Preserve the existing timeout verification while ensuring restoration also occurs when the probe rejects.Sources: Coding guidelines, Learnings
🤖 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/inference/openai-validation-session.test.ts`:
- Around line 148-177: Update the test around
probeOpenAiLikeEndpointWithValidationSession so the streaming response remains
open long enough to trigger the stalled-stream fallback, while the server
returns a valid Chat Completions response for the fallback request. Use a
deterministic test-time clock to advance past the 12-second deadline without
real waiting, then assert the public result and request sequence, removing the
setTimeout spy and timeout-call assertions.
---
Nitpick comments:
In `@src/lib/inference/openai-validation-session.test.ts`:
- Around line 162-175: Update the test around
probeOpenAiLikeEndpointWithValidationSession so timeoutSpy is always restored:
place the probe call, timeout-call inspection, and related assertions in a try
block, and move timeoutSpy.mockRestore() into finally. Preserve the existing
timeout verification while ensuring restoration also occurs when the probe
rejects.
🪄 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: 43ca0ac0-6e7a-42a1-ab25-d190e4af7df4
📒 Files selected for processing (1)
src/lib/inference/openai-validation-session.test.ts
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
senthilr-nv
approved these changes
Jul 31, 2026
senthilr-nv
left a comment
senthilr-nv
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed the Files changed for PR #7884. The substantive diff has SHA-256 2234814c35aeed8ebd3e483cae1d7438bdc23fd3de3d57e4f3ed3d941cceb120 at head 5d39f8f against base 1fc2ad1. The bounded Responses streaming probe and public-boundary fallback regression coverage preserve the intended onboarding behavior. I approve this reviewed change.
23 tasks
senthilr-nv
added a commit
that referenced
this pull request
Aug 1, 2026
<!-- markdownlint-disable MD041 --> ## Summary Adds the canonical dated changelog entry for `v0.0.100` so the maintainer release plan can verify the pre-tag documentation prerequisite. The entry summarizes the user-facing changes merged since `v0.0.99` and links to the relevant guides. ## Changes - Add `docs/changelog/2026-07-31.mdx` with the exact `## v0.0.100` heading. - Cover restored OpenClaw pairing, transactional replacement, Deep Agents Code, onboarding recovery, lifecycle cleanup, Hermes builds, host provenance, documentation, and trusted E2E evidence. - Distinguish active Docker and Kubernetes runtime-bundle enforcement from the still-inactive managed shared-state transaction foundation. ## Source Coverage The release entry maps the doc-impacting merged PRs in the `v0.0.99..main` release range to `docs/changelog/2026-07-31.mdx`: #8021, #8024, #7973, #8028, #7947, #7788, #7884, #8023, #7969, #8020, #7989, #8000, #7907, #7942, #7567, #8013, #7955, #8017, #8014, #8015, #7629, #7644, #7821, #7971, and #7991. PR #7974 was reviewed after the final rebase and excluded because it changes internal maintainer-skill attribution policy and tests only; it does not change a user-facing product or documentation surface. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [x] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates - [ ] Tests added or updated for changed behavior - [x] Existing tests cover changed behavior — justification: the changelog contract test validates the dated entry, version heading, SPDX form, and route constraints. - [ ] Tests not applicable — justification: - [x] Docs updated for user-facing behavior changes - [ ] Docs not applicable — justification: - [ ] 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: - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Documentation Writer Review - [x] Documentation writer subagent reviewed the completed changes - Result: `docs-updated` - Evidence: `docs/changelog/2026-07-31.mdx`; exact-head review passed for `6093f44f`; writing rules and documentation style reviewed; `npx vitest run test/changelog-docs.test.ts` passed 6/6; `npm run docs` passed with zero Fern errors and two generic Fern upgrade notices. - Agent: Codex Desktop <!-- docs-review-head-sha: 6093f44 --> <!-- docs-review-agents-blob-sha: 3dd7c24 --> ## DGX Station Hardware Evidence - [ ] Tested on DGX Station - Tested commit: Not applicable; no DGX Station host script changed. - Station profile/scenario: Not applicable. - Result: Not applicable. - Supporting evidence: Not applicable. ## Verification - [x] PR description includes a `Signed-off-by:` line and every commit appears as `Verified` in GitHub - [x] 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 - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — command/result or justification: `npx vitest run test/changelog-docs.test.ts` passed 6/6 at `6093f44f`. - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: Not applicable to a dated prose-only release entry. - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `npm run docs` builds without warnings (doc changes only) — validation passed with zero errors; Fern emitted two generic upgrade notices. - [x] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) — the changelog entry has the required parser-safe MDX SPDX header; dated changelog entries intentionally do not use page frontmatter. --- Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Added release notes for v0.0.100. * Documented improvements to restore pairing, sandbox replacement, onboarding recovery, lifecycle cleanup, runtime handling, build support, host readiness, and end-to-end validation. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
23 tasks
senthilr-nv
added a commit
that referenced
this pull request
Aug 1, 2026
<!-- markdownlint-disable MD041 --> ## Summary <!-- 1-3 plain sentences: what changes and why. Describe before-and-after behavior when it applies. Follow the NemoClaw Writing Guide: https://github.com/NVIDIA/NemoClaw/blob/main/WRITING.md. Do not add unrelated prose cleanup. --> Onboarding could spend 12 seconds on a stalled Responses API stream even though the calibrated full-E2E inference-phase budget is 6 seconds. This change limits that probe to the budget's existing 5-second headroom, preserves Chat Completions API fallback, and leaves the E2E budget unchanged. ## Changes <!-- List concrete changes. If this adds an abstraction, configuration, fallback, migration, or compatibility path, name the current requirement and consumer, explain why a direct change is insufficient, and identify the test that protects it. --> - Change the shared curl and native Responses streaming deadline from 12 seconds to 5 seconds. The five calibration samples measured the complete inference phase at 563–805 ms, and the accepted gate adds 5 seconds of minimum headroom for a 6,000 ms cap. - Extend the curl public-boundary test to prove that a 300-second general validation override produces a 5-second streaming request between unchanged 300-second non-streaming requests. - Update the native stalled-stream test to prove fallback after the 5-second deadline without shortening the initial Responses request. - Document the user-visible deadline and correct the v0.0.100 release entry. No new abstraction, configuration, fallback, migration, compatibility path, or supported surface is added. The existing custom-endpoint onboarding flow and Chat Completions API fallback are the current consumers. The product root cause was a source deadline introduced after the blocking phase gate without checking the two contracts together. The detection gap was that PR #7884 tested the 12-second value in isolation; the full-E2E run exercised the real stalled-stream path and measured 11,950 ms for the inference phase. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [x] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates <!-- Check one tests line and one docs line. Check other lines when applicable. Add every requested justification or approval reference. --> - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [x] Docs updated for user-facing behavior changes - [ ] Docs not applicable — justification: - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: Codex Desktop reviewed exact commit `134e9cbf77d43859bbaf84c5d02d3005ac847b5b`. All nine categories pass. The diff changes no credential, input-validation, authorization, dependency, logging, cryptography, configuration, header, policy, or SSRF boundary. Both transports retain authentication, address pinning, and the established Chat Completions API fallback. Focused tests cover the curl and native deadline boundaries. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Documentation Writer Review <!-- Required for code and documentation changes after the changes and applicable validation are complete. Keep one review checkbox and one instance of each visible or hidden field. For Evidence, list changed documentation paths. For documentation-only changes, also state that the writing rules and documentation style were reviewed. For other results, explain why no documentation change is needed or why the review is blocked. For Agent, use a consistent product and surface name, such as Codex Desktop, Codex CLI, Claude Code, or Cursor. After committing all review changes, put `git rev-parse --short HEAD` and `git rev-parse --short HEAD:AGENTS.md` in the hidden metadata below. Rerun the review and refresh that metadata after any new commit. This receipt is advisory during the data-collection pilot. --> - [x] Documentation writer subagent reviewed the completed changes - Result: `docs-updated` - Evidence: Reviewed `docs/inference/understand-provider-validation.mdx` and `docs/changelog/2026-07-31.mdx`, plus changed comments and test titles. Generated OpenClaw, Hermes, and Deep Agents variants render the 5-second deadline. Focused inference tests passed 54 tests with 1 skipped, changelog tests passed 6 tests, the docs build completed with 0 errors and 2 existing warnings, and `git diff --check` passed. - Agent: Codex Desktop <!-- docs-review-head-sha: 134e9cb --> <!-- docs-review-agents-blob-sha: 3dd7c24 --> ## DGX Station Hardware Evidence <!-- Required only when scripts/prepare-dgx-station-host.sh changes. Maintainers must review the linked evidence before approving or merging. This is human-reviewed evidence, not authenticated hardware provenance. Exceptional bypasses use existing repository governance and must be documented on the PR. --> - [ ] Tested on DGX Station - Tested commit: Not applicable; `scripts/prepare-dgx-station-host.sh` is unchanged. - Station profile/scenario: Not applicable. - Result: Not applicable. - Supporting evidence: Not applicable. ## Verification <!-- Check each applicable item only when supported by the requested evidence. Run targeted tests once per relevant change set and rerun after later edits or hook autofixes that can affect the tested behavior. Do not rerun hook-covered checks. --> - [x] PR description includes a `Signed-off-by:` line and every commit appears as `Verified` in GitHub - [x] 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 - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — `npx vitest run --project cli src/lib/inference/probe-http-helpers.test.ts src/lib/inference/onboard-probes.test.ts src/lib/inference/openai-validation-session.test.ts src/lib/inference/openai-validation-session-fallback.test.ts`: 4 files passed, 54 tests passed, 1 skipped. `npx vitest run test/changelog-docs.test.ts`: 1 file and 6 tests passed. - [ ] Applicable broad gate passed — not applicable; this changes one bounded onboarding deadline, focused regression coverage, and matching documentation. Required CI runs the broader suite. - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `npm run docs` builds without warnings (doc changes only) — the build completed with 0 errors and 2 existing warnings. - [x] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) — no new pages. --- <!-- DCO sign-off is required in this PR description, and every commit must appear as Verified in GitHub. Run: git config user.name && git config user.email --> Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Improvements** * Compatible-endpoint onboarding now completes streaming validation after up to 5 seconds instead of 12. * Chat Completions fallback remains available when no Responses API streaming event is received. * **Documentation** * Updated validation guidance to reflect the five-second streaming wait and fallback behavior. * **Tests** * Expanded coverage for timeout handling, fallback requests, retries, and configurable validation timeouts. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Onboarding could spend the full validation budget on a custom endpoint that opens a Responses API stream without
response.output_text.delta. The streaming validation request now usesmin(existing budget, 12 seconds)in both transports, then preserves the existing Chat Completions API fallback.Related Issue
Fixes #7792
Changes
STREAMING_EVENT_PROBE_MAX_SECONDSandgetStreamingEventProbeCurlArgs()as the shared timeout source for the curl and native validation-session paths.No new configuration, fallback, migration, compatibility path, or supported surface is added. The existing custom-endpoint validation flow and Chat Completions API fallback are the current consumers.
Type of Change
Quality Gates
5d39f8f14965158c606ab3a9e904bb920a479ab0on base SHA1fc2ad15d1903864562c4f6553324e51987ffed5. All nine security categories pass. The diff adds no credential, authorization, dependency, cryptography, policy, header, logging, or server-side request forgery change. Existing DNS pinning and trusted configuration-file boundaries remain unchanged. Regression tests cover both transports and prove the deadline never increases the current budget.Product scope is approved independently of GitHub merge state. The change corrects the existing documented custom-endpoint validation behavior and does not create a supported surface. Issue #7792 is a native
Bugin the NemoClaw Development Tracker; the existing behavior is documented indocs/inference/understand-provider-validation.mdxanddocs/inference/choose-compatible-inference-api.mdx.Exact review evidence:
5d39f8f14965158c606ab3a9e904bb920a479ab01fc2ad15d1903864562c4f6553324e51987ffed52234814c35aeed8ebd3e483cae1d7438bdc23fd3de3d57e4f3ed3d941cceb12052f0574fd04620be656bb05a8c3909f880fc08b4. PR Review Advisor native-timeout coverage warning resolved by018c3daedff289e98591a1d0cad424868aceacf5. CodeRabbit public-boundary coverage and spy-restoration findings resolved bye3065d966517d40add5b9cc174211510a85b30f5. The codebase-growth guardrail finding on that test resolved by76066f0bd3dfbf5b932b5dba9c9dbf98bf031ea1.Documentation Writer Review
no-docs-needed5d39f8f14found no documentation or Fern changes. Existing pages already document the required streaming event and Chat Completions API fallback. The deadline is internal and non-configurable. Changed comments and test titles pass the writing rules. Focused tests passed 54 tests with 1 skipped; the conditional scanner, CLI build, type-checking, diff checks, and canonical-base hooks passed.DGX Station Hardware Evidence
scripts/prepare-dgx-station-host.shis unchanged, and this review claims no hardware evidence.Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailable — canonical-base equivalents passed withupstream/mainas the required base.npx vitest run --project cli src/lib/inference/probe-http-helpers.test.ts src/lib/inference/onboard-probes.test.ts src/lib/inference/openai-validation-session.test.ts src/lib/inference/openai-validation-session-fallback.test.ts: 4 files passed, 54 tests passed, 1 skipped.npm run docsbuilds without warnings (doc changes only) — not applicableSigned-off-by: Hung Le hple@nvidia.com