Skip to content

fix(onboard): retry transient inference smoke - #5932

Merged
cv merged 3 commits into
mainfrom
codex/retry-hosted-smoke
Jun 28, 2026
Merged

fix(onboard): retry transient inference smoke#5932
cv merged 3 commits into
mainfrom
codex/retry-hosted-smoke

Conversation

@cv

@cv cv commented Jun 28, 2026

Copy link
Copy Markdown
Collaborator

Summary

Retries transient compatible-endpoint sandbox smoke failures during onboarding and aligns the retained Hermes direct hosted probe with its already-retrying Vitest counterpart. This prevents short provider/proxy propagation windows and empty hosted responses from masking the behavior under test while retaining a strict, bounded failure budget.

Changes

  • Retry the production compatible-endpoint sandbox smoke up to three times with bounded backoff, including curl failures and non-JSON gateway responses.
  • Preserve the larger-token retry used for reasoning-only responses and fail after the configured attempt budget.
  • Add executable recovery and exhaustion tests using a fake 504 Gateway Time-out response.
  • Retry the retained Hermes direct hosted PONG probe three times, matching the live Vitest lane.

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: the live Hermes Vitest lane already exercises three-attempt direct and sandbox hosted inference retries.
  • Tests not applicable — justification:
  • Docs updated for user-facing behavior changes
  • Docs not applicable — justification: bounded resilience for existing validation probes; no user-facing configuration or workflow changes.
  • 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: retry scope is limited to the existing authenticated validation request; provider configuration, credential binding, redaction, and strict final failure behavior are unchanged.
  • 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:

  • npx vitest run --project cli src/lib/onboard/compatible-endpoint-smoke.test.ts (8 tests passed)
  • npx biome check src/lib/onboard/compatible-endpoint-smoke.ts src/lib/onboard/compatible-endpoint-smoke.test.ts
  • bash -n test/e2e/test-hermes-e2e.sh
  • npx prek run shellcheck --files test/e2e/test-hermes-e2e.sh
  • npm run typecheck:cli
  • git diff --check

Runtime evidence on c4ad10eceb811a8803700237a86978ac93142370:

  • Full Nightly run 28324129704: 66 successful, 4 skipped, and two failures. Hermes dashboard had one empty direct hosted response while all dashboard and routed checks passed. Token rotation had one same-token reuse smoke receive an HTML 504 while all rotation assertions passed.
  • Isolated Hermes dashboard run 28325346077 passed on the same SHA.
  • Isolated token rotation run 28325550172 reproduced HTML 504 responses in same-token reuse phases 5 and 7 while all rebuild/rotation phases passed, confirming the bounded sandbox-smoke retry is required independently of full-gate load.

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

Summary by CodeRabbit

  • New Features
    • Added configurable retry behavior for compatible-endpoint smoke checks (custom attempts and retry delay), including improved shell-script environment parameters.
  • Bug Fixes
    • Made smoke checks more resilient to transient non-JSON gateway errors and updated retry decisioning for 5xx conditions and max-token scenarios.
    • Updated sandbox smoke command timeout calculation to match retry and request settings.
    • Improved hosted inference endpoint e2e test retries with clearer “PONG not found” failure output.
  • Tests
    • Expanded sandbox smoke expectations to validate retry counts, timeouts, and curl invocation across success and multiple failure modes.

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

coderabbitai Bot commented Jun 28, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

Adds configurable retry parameters to the compatible-endpoint sandbox smoke script, changes the generated shell flow to loop over bounded attempts, updates timeout and non-JSON diagnostics, and adjusts unit and e2e tests to cover the retry behavior.

Smoke Script Retry Loop

Layer / File(s) Summary
Options type and helper
src/lib/onboard/compatible-endpoint-smoke.ts
CompatibleEndpointSandboxSmokeScriptOptions gains optional attempts and retryDelaySeconds fields; nonNegativeInt normalizes retry delay values, and StdioOptions is imported as a type.
Retry loop generation
src/lib/onboard/compatible-endpoint-smoke.ts
buildCompatibleEndpointSandboxSmokeScript computes normalized retry settings, derives the sandbox command timeout, emits SMOKE_ATTEMPTS/SMOKE_REQUEST_TIMEOUT_SECONDS/SMOKE_RETRY_DELAY_SECONDS, changes curl failure handling to return codes, expands non-JSON diagnostics, and replaces the fixed smoke flow with a bounded loop.
Unit tests for retry behavior
src/lib/onboard/compatible-endpoint-smoke.test.ts
Adds coverage for the host timeout wiring, asserts the generated smoke constants, updates existing smoke test calls to pass explicit retry options, and adds retry success and exhaustion cases for transient and permanent failures.
E2E hosted inference retry
test/e2e/test-hermes-e2e.sh
Replaces the single-attempt hosted inference curl check with a 3-attempt loop using per-attempt max_tokens, parses each response, retries on non-PONG output, and simplifies the final assertion.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested labels

area: onboarding, bug-fix

Suggested reviewers

  • jyaunches
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.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 clearly matches the main change: bounded retries for transient onboarding inference smoke failures.
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 docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/retry-hosted-smoke

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

@github-code-quality

github-code-quality Bot commented Jun 28, 2026

Copy link
Copy Markdown
Contributor

Code Coverage Overview

Languages: TypeScript

TypeScript / code-coverage/plugin

The overall coverage in the codex/retry-hosted-s... 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 codex/retry-hosted-s... cb65f81 +/-
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 codex/retry-hosted-s... branch is 68%. Coverage data for the main branch is not yet available.

Show a code coverage summary of the most covered files.
File main codex/retry-hosted-s... cb65f81 +/-
src/lib/actions...all/run-plan.ts 80%
src/lib/state/o...oard-session.ts 78%
src/lib/actions...dbox/rebuild.ts 74%
src/lib/state/sandbox.ts 72%
src/lib/shields/index.ts 70%
src/lib/onboard/preflight.ts 69%
src/lib/actions...licy-channel.ts 59%
src/lib/onboard...er-gpu-patch.ts 59%
src/lib/policy/index.ts 52%
src/lib/onboard.ts 20%

Updated June 28, 2026 15:26 UTC
Code Coverage is in Public Preview. Learn more and provide us with your feedback.

@github-actions

github-actions Bot commented Jun 28, 2026

Copy link
Copy Markdown
Contributor

E2E Advisor Recommendation

Required E2E: messaging-compatible-endpoint-e2e, hermes-e2e
Optional E2E: inference-routing-e2e, cloud-onboard-e2e

Dispatch hint: messaging-compatible-endpoint-e2e,hermes-e2e

Auto-dispatched E2E: messaging-compatible-endpoint-e2e via nightly-e2e.yaml at cb65f819621b0d73fe78768e57aced8714032daanightly run

Workflow run

Full advisor summary

E2E Recommendation Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required E2E

  • messaging-compatible-endpoint-e2e (medium): Direct coverage for the changed product path: onboards OpenClaw with a compatible OpenAI-style endpoint plus Telegram, asserts onboarding ran the compatible endpoint sandbox smoke check, verifies openclaw.json uses the managed inference.local provider, and exercises sandbox-side/agent inference through the compatible endpoint.
  • hermes-e2e (medium): The PR modifies test/e2e/test-hermes-e2e.sh. Run the primary Hermes E2E job to validate the changed direct hosted inference retry loop and ensure the script still completes the install/onboard/health/live-inference Hermes user flow.

Optional E2E

  • inference-routing-e2e (medium): Adjacent confidence for the inference.local gateway routing layer that the compatible endpoint smoke relies on, though messaging-compatible-endpoint-e2e is the more targeted required coverage.
  • cloud-onboard-e2e (medium): Optional broader hosted onboarding confidence because the touched code is under src/lib/onboard, but the modified branch is gated to compatible-endpoint plus messaging and is more directly covered by messaging-compatible-endpoint-e2e.

New E2E recommendations

  • compatible endpoint sandbox smoke retry behavior (medium): Existing messaging-compatible-endpoint-e2e verifies the happy path and that the smoke check ran, but it does not appear to force the onboard-time smoke to receive a transient non-JSON/5xx gateway response followed by success. The retry/redaction behavior changed here is therefore primarily unit-tested.
    • Suggested test: Extend the compatible endpoint mock path used by messaging-compatible-endpoint-e2e with an opt-in mode that returns one HTML 504/non-JSON response for the onboard smoke request, then a valid chat completion, and assert onboarding succeeds without leaking secret-shaped response body content.

Dispatch hint

  • Workflow: nightly-e2e.yaml
  • jobs input: messaging-compatible-endpoint-e2e,hermes-e2e

@github-actions

github-actions Bot commented Jun 28, 2026

Copy link
Copy Markdown
Contributor

Vitest E2E Scenario Recommendation

Required Vitest E2E scenarios: messaging-compatible-endpoint-vitest
Optional Vitest E2E scenarios: None

Dispatch required Vitest E2E scenarios:

  • gh workflow run e2e-vitest-scenarios.yaml --ref <pr-head-ref> --field jobs=messaging-compatible-endpoint-vitest

Workflow run

Full Vitest E2E advisor summary

Vitest E2E Scenario Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required Vitest E2E scenarios

  • messaging-compatible-endpoint-vitest: The PR changes the compatible-endpoint sandbox smoke helper used during OpenClaw compatible-endpoint onboarding with messaging. The wired free-standing messaging-compatible-endpoint Vitest job exercises that path with a fake OpenAI-compatible endpoint, Telegram configuration, inference.local sandbox smoke, and OpenClaw agent-turn validation.
    • Dispatch: gh workflow run e2e-vitest-scenarios.yaml --ref <pr-head-ref> --field jobs=messaging-compatible-endpoint-vitest

Optional Vitest E2E scenarios

  • None.

Relevant changed files

  • src/lib/onboard/compatible-endpoint-smoke.ts

@github-actions

github-actions Bot commented Jun 28, 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: Compatible endpoint sandbox smoke retry and tolerant non-JSON handling.
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: Compatible endpoint sandbox smoke retry and tolerant non-JSON handling
  • PRA-2 Resolve or justify: Curl-exit retry branch remains untested in src/lib/onboard/compatible-endpoint-smoke.ts:356
  • 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: Curl-exit retry branch remains untested
  • PRA-T5 Add or justify test follow-up: Acceptance clause
  • 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 tests src/lib/onboard/compatible-endpoint-smoke.ts:356 Add a local Vitest case beside the new non-JSON smoke tests where the fake `curl` exits non-zero on the first call and emits a valid PONG JSON response on the second call; assert status 0, retry diagnostics, and a call count of 2.
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: Compatible endpoint sandbox smoke retry and tolerant non-JSON handling

  • 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: Non-JSON 504 recovery/exhaustion, reasoning-only recovery/failure, host timeout budgeting, and response-body non-leak are covered. The curl non-zero recovery regression is missing and is covered by the current finding.
  • 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: `compatible-endpoint-smoke.ts` validates `openclaw.json`, then retries the inference smoke up to three times with bounded curl timeouts and redacted non-JSON diagnostics. The current test suite does not yet prove the curl process failure branch.

PRA-2 Resolve/justify — Curl-exit retry branch remains untested

  • Location: src/lib/onboard/compatible-endpoint-smoke.ts:356
  • Category: tests
  • Problem: The generated sandbox smoke script now treats a non-zero `curl` exit as retryable by returning from `run_smoke_request` and continuing the outer attempt loop. The changed tests cover non-JSON 504 recovery, bounded 504 exhaustion, permanent JSON validation failures, reasoning-only retries, and host timeout budgeting, but none makes fake `curl` exit non-zero before a later successful JSON response. This leaves one explicit acceptance clause and the source-of-truth regression requirement only partially covered.
  • Impact: A future edit could accidentally restore immediate exit-on-curl-failure behavior or break retrying curl timeouts such as exit 28 while the current test suite still passes, leaving onboarding flaky for the transient sandbox/proxy failure mode this PR is intended to tolerate.
  • Recommended action: Add a local Vitest case beside the new non-JSON smoke tests where the fake `curl` exits non-zero on the first call and emits a valid PONG JSON response on the second call; assert status 0, retry diagnostics, and a call count of 2.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read the `run_smoke_request || { status=$?; request_failed=1; }` branch and retry loop in `src/lib/onboard/compatible-endpoint-smoke.ts`, then compare the current test names in `src/lib/onboard/compatible-endpoint-smoke.test.ts`; the tests exercise response bodies but not a non-zero curl process exit.
  • Missing regression test: Add `retries a transient curl exit before succeeding` using `writeFakeCurl` with `if [ "$count" -eq 1 ]; then exit 28; else printf '%s\n' '{"choices":[{"message":{"content":"PONG"},"finish_reason":"stop"}]}'; fi`, `attempts: 3`, and `retryDelaySeconds: 0`; assert `result.status === 0`, stderr includes `curl exit 28` and `smoke attempt 1/3 failed; retrying in 0s`, and `curl-calls` is `2`.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read the `run_smoke_request || { status=$?; request_failed=1; }` branch and retry loop in `src/lib/onboard/compatible-endpoint-smoke.ts`, then compare the current test names in `src/lib/onboard/compatible-endpoint-smoke.test.ts`; the tests exercise response bodies but not a non-zero curl process exit.
  • Evidence: `run_smoke_request` now returns the curl rc instead of exiting, and the outer loop retries request failures. Current added tests are named `retries a transient non-JSON gateway response`, `does not retry a permanent JSON response validation failure`, and `fails after the bounded transient retry budget`; none simulates `curl` itself exiting non-zero.

💡 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.
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 — Vitest: `retries a transient curl exit before succeeding` for `buildCompatibleEndpointSandboxSmokeScript`, with fake curl exit 28 on attempt 1 and JSON PONG on attempt 2.. The changed behavior crosses generated shell, Python parsing, curl, sandbox execution, proxy routing, and hosted inference boundaries. Local tests cover most generated-script branches, but one explicit retry branch is untested and runtime validation is advisable for the real sandbox/network path.
  • PRA-T2 Runtime validation — Runtime/integration: compatible-endpoint sandbox smoke succeeds within the 220s host timeout when earlier attempts hit transient curl/network failures and a later attempt returns JSON PONG.. The changed behavior crosses generated shell, Python parsing, curl, sandbox execution, proxy routing, and hosted inference boundaries. Local tests cover most generated-script branches, but one explicit retry branch is untested and runtime validation is advisable for the real sandbox/network path.
  • PRA-T3 Runtime validation — Shell harness if available: Hermes direct hosted probe retries an empty or non-PONG first response and succeeds when a later attempt returns PONG, without logging response body or credential material.. The changed behavior crosses generated shell, Python parsing, curl, sandbox execution, proxy routing, and hosted inference boundaries. Local tests cover most generated-script branches, but one explicit retry branch is untested and runtime validation is advisable for the real sandbox/network path.
  • PRA-T4 Curl-exit retry branch remains untested — Add a local Vitest case beside the new non-JSON smoke tests where the fake `curl` exits non-zero on the first call and emits a valid PONG JSON response on the second call; assert status 0, retry diagnostics, and a call count of 2.
  • PRA-T5 Acceptance clause — Retry the production compatible-endpoint sandbox smoke up to three times with bounded backoff, including curl failures and non-JSON gateway responses. — add test evidence or identify existing coverage. Code sets `COMPATIBLE_ENDPOINT_SMOKE_ATTEMPTS = 3`, `COMPATIBLE_ENDPOINT_SMOKE_REQUEST_TIMEOUT_SECONDS = 60`, `COMPATIBLE_ENDPOINT_SMOKE_RETRY_DELAY_SECONDS = 5`, and a 220s host timeout; the generated loop retries non-JSON gateway 5xx responses and request failures. Tests cover non-JSON recovery/exhaustion and timeout budgeting, but there is no regression test for a non-zero curl exit followed by success.
  • PRA-T6 Acceptance clause — Tests added or updated for changed behavior — add test evidence or identify existing coverage. Tests were added or updated for timeout budgeting, non-JSON recovery/exhaustion, permanent JSON no-retry, and reasoning-only retry behavior. The curl non-zero retry behavior remains uncovered.
  • PRA-T7 Acceptance clause — Existing tests cover changed behavior — justification: the live Hermes Vitest lane already exercises three-attempt direct and sandbox hosted inference retries. — add test evidence or identify existing coverage. PR-provided runtime/job claims are untrusted review evidence and external E2E status is out of scope for this advisor surface. Static inventory found local tests for the TypeScript smoke helper and the changed E2E shell script, but no named local test for the Hermes direct hosted retry loop.
  • PRA-T8 Acceptance clause — Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: retry scope is limited to the existing authenticated validation request; provider configuration, credential binding, redaction, and strict final failure behavior are unchanged. — add test evidence or identify existing coverage. Security review found no direct sandbox escape, SSRF, policy bypass, or credential leak regression, and diagnostics avoid logging non-JSON response bodies. The remaining security-testing warning is the missing curl non-zero retry regression at the sandbox/network boundary.
Since last review details

Current findings, using the urgency labels above:

PRA-1 Resolve/justify — Source-of-truth review needed: Compatible endpoint sandbox smoke retry and tolerant non-JSON handling

  • 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: Non-JSON 504 recovery/exhaustion, reasoning-only recovery/failure, host timeout budgeting, and response-body non-leak are covered. The curl non-zero recovery regression is missing and is covered by the current finding.
  • 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: `compatible-endpoint-smoke.ts` validates `openclaw.json`, then retries the inference smoke up to three times with bounded curl timeouts and redacted non-JSON diagnostics. The current test suite does not yet prove the curl process failure branch.

PRA-2 Resolve/justify — Curl-exit retry branch remains untested

  • Location: src/lib/onboard/compatible-endpoint-smoke.ts:356
  • Category: tests
  • Problem: The generated sandbox smoke script now treats a non-zero `curl` exit as retryable by returning from `run_smoke_request` and continuing the outer attempt loop. The changed tests cover non-JSON 504 recovery, bounded 504 exhaustion, permanent JSON validation failures, reasoning-only retries, and host timeout budgeting, but none makes fake `curl` exit non-zero before a later successful JSON response. This leaves one explicit acceptance clause and the source-of-truth regression requirement only partially covered.
  • Impact: A future edit could accidentally restore immediate exit-on-curl-failure behavior or break retrying curl timeouts such as exit 28 while the current test suite still passes, leaving onboarding flaky for the transient sandbox/proxy failure mode this PR is intended to tolerate.
  • Recommended action: Add a local Vitest case beside the new non-JSON smoke tests where the fake `curl` exits non-zero on the first call and emits a valid PONG JSON response on the second call; assert status 0, retry diagnostics, and a call count of 2.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read the `run_smoke_request || { status=$?; request_failed=1; }` branch and retry loop in `src/lib/onboard/compatible-endpoint-smoke.ts`, then compare the current test names in `src/lib/onboard/compatible-endpoint-smoke.test.ts`; the tests exercise response bodies but not a non-zero curl process exit.
  • Missing regression test: Add `retries a transient curl exit before succeeding` using `writeFakeCurl` with `if [ "$count" -eq 1 ]; then exit 28; else printf '%s\n' '{"choices":[{"message":{"content":"PONG"},"finish_reason":"stop"}]}'; fi`, `attempts: 3`, and `retryDelaySeconds: 0`; assert `result.status === 0`, stderr includes `curl exit 28` and `smoke attempt 1/3 failed; retrying in 0s`, and `curl-calls` is `2`.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read the `run_smoke_request || { status=$?; request_failed=1; }` branch and retry loop in `src/lib/onboard/compatible-endpoint-smoke.ts`, then compare the current test names in `src/lib/onboard/compatible-endpoint-smoke.test.ts`; the tests exercise response bodies but not a non-zero curl process exit.
  • Evidence: `run_smoke_request` now returns the curl rc instead of exiting, and the outer loop retries request failures. Current added tests are named `retries a transient non-JSON gateway response`, `does not retry a permanent JSON response validation failure`, and `fails after the bounded transient retry budget`; none simulates `curl` itself exiting non-zero.

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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/onboard/compatible-endpoint-smoke.ts`:
- Around line 349-360: The smoke test retry flow in compatible-endpoint-smoke
should distinguish transient non-JSON gateway responses from terminal validation
failures instead of treating every non-zero from check_response the same. Update
check_response to return a distinct retryable status only when can_retry is true
and the body is non-JSON, and ensure the outer attempt loop around
check_response/$status exits immediately for permanent JSON-shape/content
validation failures rather than sleeping and retrying. Use the existing
check_response, can_retry, and status handling in compatible-endpoint-smoke to
keep the retry loop limited to genuinely transient failures.

In `@test/e2e/test-hermes-e2e.sh`:
- Around line 625-638: The direct API retry path is losing the useful raw
gateway response when parse_chat_content fails, so the exhausted failure message
can end up empty. In the direct API loop, keep api_response as the fallback
source and use it in the final fail branch when api_content is empty or parsing
fails, so the retry exhaustion log still shows the raw 504/body text. Update the
logic around parse_chat_content, api_content, and the final pass/fail checks to
preserve that payload for debugging.
🪄 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: 8364524c-0c41-4083-ad87-6debb404f7a1

📥 Commits

Reviewing files that changed from the base of the PR and between c4ad10e and 30ca8fc.

📒 Files selected for processing (3)
  • src/lib/onboard/compatible-endpoint-smoke.test.ts
  • src/lib/onboard/compatible-endpoint-smoke.ts
  • test/e2e/test-hermes-e2e.sh

Comment thread src/lib/onboard/compatible-endpoint-smoke.ts
Comment thread test/e2e/test-hermes-e2e.sh Outdated
@github-actions

github-actions Bot commented Jun 28, 2026

Copy link
Copy Markdown
Contributor

PR Review Advisor (Nemotron Ultra) — Changes requested

Merge posture: Do not merge yet
Primary next action: Fix PRA-2: Missing test for curl-level transient failures despite PR claiming 'including curl failures'; then add or justify PRA-T1.
Open items: 1 required · 8 warnings · 3 suggestions · 8 test follow-ups
Since last review: 0 prior items resolved · 11 still apply · 3 new items found

Action checklist

  • PRA-2 Fix: Missing test for curl-level transient failures despite PR claiming 'including curl failures' in src/lib/onboard/compatible-endpoint-smoke.test.ts:1
  • PRA-1 Resolve or justify: Source-of-truth review needed: Retry loop in buildCompatibleEndpointSandboxSmokeScript (lines 354-382)
  • PRA-3 Resolve or justify: curl stderr printed unredacted — may leak Authorization headers on 401/403 in src/lib/onboard/compatible-endpoint-smoke.ts:304
  • PRA-4 Resolve or justify: Retry loop retries on ALL non-zero curl exit codes including non-transient ones in src/lib/onboard/compatible-endpoint-smoke.ts:364
  • PRA-5 Resolve or justify: Retry loop is undocumented workaround for external gateway instability in src/lib/onboard/compatible-endpoint-smoke.ts:354
  • PRA-6 Resolve or justify: Fragile HTML status extraction — regex only matches <title>/<h1> with 3-digit code in src/lib/onboard/compatible-endpoint-smoke.ts:330
  • PRA-7 Resolve or justify: Monolith growth: compatible-endpoint-smoke.ts grew by 61 lines (351→412), exceeds 20-line threshold in src/lib/onboard/compatible-endpoint-smoke.ts:1
  • PRA-8 Resolve or justify: Magic number COMPATIBLE_ENDPOINT_SMOKE_COMMAND_OVERHEAD_SECONDS = 30 undocumented in src/lib/onboard/compatible-endpoint-smoke.ts:38
  • PRA-9 Resolve or justify: Fixed 5s retry delay in smoke script vs E2E exponential backoff (5s, 10s, 15s) in src/lib/onboard/compatible-endpoint-smoke.ts:37
  • 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: Missing negative test for non-retry on non-transient curl exit codes
  • PRA-T7 Add or justify test follow-up: Acceptance clause
  • PRA-T8 Add or justify test follow-up: Retry loop in buildCompatibleEndpointSandboxSmokeScript (lines 354-382)
  • PRA-10 In-scope improvement: E2E retry hardcodes max_tokens=1024/256 instead of using named constants matching smoke script in test/e2e/test-hermes-e2e.sh:620
  • PRA-11 In-scope improvement: nonNegativeInt helper simple enough to inline (used once) in src/lib/onboard/compatible-endpoint-smoke.ts:55
  • PRA-12 In-scope improvement: Missing negative test for non-retry on non-transient curl exit codes in src/lib/onboard/compatible-endpoint-smoke.test.ts:1

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 Required acceptance src/lib/onboard/compatible-endpoint-smoke.test.ts:1 Add test cases simulating curl exit 28 (timeout), exit 7 (connection refused), exit 6 (DNS failure) on first attempt, then valid JSON on second. Modify writeFakeCurl or add a variant that can simulate specific exit codes. Verify loop retries and succeeds.
PRA-3 Resolve/justify security src/lib/onboard/compatible-endpoint-smoke.ts:304 Redact sensitive patterns (Authorization: Bearer, apiKey, etc.) from curl stderr before printing, or suppress curl stderr entirely for failed requests since the Python handler already logs structured diagnostics (byte count, HTTP status).
PRA-4 Resolve/justify correctness src/lib/onboard/compatible-endpoint-smoke.ts:364 Option A: Map known transient curl exit codes (6, 7, 28, 52, 56) to a retryable code in run_smoke_request, keep loop simple. Option B: Check for transient codes explicitly in loop condition. Option A is cleaner.
PRA-5 Resolve/justify architecture src/lib/onboard/compatible-endpoint-smoke.ts:354 Add a comment in buildCompatibleEndpointSandboxSmokeScript or at the top of the generated shell script noting: 'WORKAROUND: Retries transient gateway failures (504, timeouts, curl network errors). Remove when gateway SLA guarantees no transient failures during propagation. See NemoClaw #XXXX.'
PRA-6 Resolve/justify correctness src/lib/onboard/compatible-endpoint-smoke.ts:330 Also check for 5xx in response body more broadly (e.g., regex for '5[0-9]{2}' anywhere in first 1000 chars), or accept that only HTML gateway errors with standard markup are retryable. Document the assumption.
PRA-7 Resolve/justify architecture src/lib/onboard/compatible-endpoint-smoke.ts:1 Extract retry loop logic (constants, loop structure, attempt accounting) to a separate module or helper function. Keep generated script focused on payload/response handling.
PRA-8 Resolve/justify correctness src/lib/onboard/compatible-endpoint-smoke.ts:38 Add comment explaining the 30s overhead breakdown or derive empirically.
PRA-9 Resolve/justify correctness src/lib/onboard/compatible-endpoint-smoke.ts:37 Consider exponential backoff in smoke script: sleep $((SMOKE_RETRY_DELAY_SECONDS * attempt)) or document that operators should override retryDelaySeconds for production gateways.
PRA-10 Improvement correctness test/e2e/test-hermes-e2e.sh:620 Extract INITIAL_MAX_TOKENS=256 and RETRY_MAX_TOKENS=1024 as variables at top of Phase 5 section, mirroring smoke script defaults.
PRA-11 Improvement scope src/lib/onboard/compatible-endpoint-smoke.ts:55 Inline the helper at call site: const retryDelaySeconds = Number.isFinite(options.retryDelaySeconds) ? Math.max(0, Math.floor(options.retryDelaySeconds)) : COMPATIBLE_ENDPOINT_SMOKE_RETRY_DELAY_SECONDS;
PRA-12 Improvement tests src/lib/onboard/compatible-endpoint-smoke.test.ts:1 Add test: 'does not retry on non-transient curl exit (e.g., exit 3 malformed URL)' — verify loop exits immediately on non-transient code. Requires fake curl helper that can simulate exit codes.

🚨 Required before merge

Address these before merging unless a maintainer explicitly overrides the advisor with rationale.

PRA-2 Required — Missing test for curl-level transient failures despite PR claiming 'including curl failures'

  • Location: src/lib/onboard/compatible-endpoint-smoke.test.ts:1
  • Category: acceptance
  • Problem: PR description and commit messages state retries cover 'curl failures and non-JSON gateway responses.' Implementation retries on ALL non-zero curl exit codes, but the fake curl helper (writeFakeCurl) only controls stdout body via ${bodyForCall}, not exit codes. No test verifies that curl timeout (exit 28), connection refused (exit 7), or DNS failure (exit 6) actually trigger a retry and succeed.
  • Impact: The claimed 'curl failures' retry behavior is untested. If the fake helper or implementation has a bug handling curl exit codes, transient network failures would fail onboarding silently without retry.
  • Required action: Add test cases simulating curl exit 28 (timeout), exit 7 (connection refused), exit 6 (DNS failure) on first attempt, then valid JSON on second. Modify writeFakeCurl or add a variant that can simulate specific exit codes. Verify loop retries and succeeds.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Search for 'exit 28' or 'timeout' or 'connection refused' or 'DNS' in compatible-endpoint-smoke.test.ts — no matches. The writeFakeCurl helper at line 47 only controls stdout body, not exit codes.
  • Missing regression test: Add test: 'retries on curl timeout (exit 28) then succeeds'. Add test: 'retries on curl connection refused (exit 7) then succeeds'. Add test: 'retries on curl DNS failure (exit 6) then succeeds'.
  • Done when: The required change is committed and verification passes: Search for 'exit 28' or 'timeout' or 'connection refused' or 'DNS' in compatible-endpoint-smoke.test.ts — no matches. The writeFakeCurl helper at line 47 only controls stdout body, not exit codes.
  • Evidence: PR body: 'Retry the production compatible-endpoint sandbox smoke up to three times with bounded backoff, including curl failures and non-JSON gateway responses.' Implementation lines 364 retries on any non-zero status. Test file has no curl exit code simulation.
Review findings by urgency: 1 required fix, 8 items to resolve/justify, 3 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: Retry loop in buildCompatibleEndpointSandboxSmokeScript (lines 354-382)

  • 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: Tests 'retries a transient non-JSON gateway response' and 'fails after bounded transient retry budget' cover gateway-level transients. MISSING: curl-level transient tests (exit 28/7/6) — see PRA-2 blocker
  • 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: PR body: 'short provider/proxy propagation windows and empty hosted responses.' CI shows HTML 504 responses. No code comment marks as workaround.

PRA-3 Resolve/justify — curl stderr printed unredacted — may leak Authorization headers on 401/403

  • Location: src/lib/onboard/compatible-endpoint-smoke.ts:304
  • Category: security
  • Problem: The run_smoke_request function prints curl stderr directly with 'cat "$error_file" >&2'. If curl includes credentials (e.g., Authorization headers) in error output for failed requests, they would flow through sandbox stderr before host-side redaction.
  • Impact: If inference.local returns 401/403, curl may write 'Authorization: Bearer <token>' to stderr. This prints directly to sandbox stderr, captured by host after execution, but redact() only runs on combined stdout+stderr after the fact. If sandbox logs are persisted or forwarded before redaction, credentials leak.
  • Recommended action: Redact sensitive patterns (Authorization: Bearer, apiKey, etc.) from curl stderr before printing, or suppress curl stderr entirely for failed requests since the Python handler already logs structured diagnostics (byte count, HTTP status).
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Lines 304-310: run_smoke_request prints raw curl stderr via 'cat "$error_file" >&2'.
  • Missing regression test: Add test verifying curl stderr with Authorization header does not leak to output.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Lines 304-310: run_smoke_request prints raw curl stderr via 'cat "$error_file" >&2'.
  • Evidence: run_smoke_request at lines 304-310 prints raw curl stderr. Test at line 187 verifies HTML body secret not leaked, but curl stderr path untested.

PRA-4 Resolve/justify — Retry loop retries on ALL non-zero curl exit codes including non-transient ones

  • Location: src/lib/onboard/compatible-endpoint-smoke.ts:364
  • Category: correctness
  • Problem: The retry loop retries on any non-zero status from run_smoke_request. Curl exit codes 1 (unsupported protocol), 2 (init failed), 3 (malformed URL), 4 (feature unsupported), 5 (proxy), 8 (weird reply) are non-transient but trigger retry. Only 6 (DNS), 7 (connection), 28 (timeout), 52 (empty reply), 56 (recv failure) are transient. Non-transient failures (e.g., malformed inference URL, unsupported protocol) will consume retry budget and delay final failure, masking configuration errors.
  • Impact: Configuration errors like malformed URLs or unsupported protocols will waste retry budget (up to 3 attempts × 5s delay = 15s) before failing, delaying feedback and masking root cause.
  • Recommended action: Option A: Map known transient curl exit codes (6, 7, 28, 52, 56) to a retryable code in run_smoke_request, keep loop simple. Option B: Check for transient codes explicitly in loop condition. Option A is cleaner.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect run_smoke_request function at lines 298-310: it returns raw curl exit code. Loop at lines 354-382 retries on any status != 0 when request_failed=1.
  • Missing regression test: Add test: 'does not retry on non-transient curl exit (e.g., exit 3 malformed URL)' — verify loop exits immediately on non-transient code.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect run_smoke_request function at lines 298-310: it returns raw curl exit code. Loop at lines 354-382 retries on any status != 0 when request_failed=1.
  • Evidence: run_smoke_request returns raw curl exit code. Loop condition at line 364: 'if [ "$status" -eq 0 ]' then check_response, else if request_failed=1 retry on any non-zero.

PRA-5 Resolve/justify — Retry loop is undocumented workaround for external gateway instability

  • Location: src/lib/onboard/compatible-endpoint-smoke.ts:354
  • Category: architecture
  • Problem: The retry loop (lines 354-382) handles transient gateway failures but has no comment marking it as a workaround with removal condition. Source-of-truth review needed.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear. Future maintainers may not know this is temporary infrastructure compensation.
  • Recommended action: Add a comment in buildCompatibleEndpointSandboxSmokeScript or at the top of the generated shell script noting: 'WORKAROUND: Retries transient gateway failures (504, timeouts, curl network errors). Remove when gateway SLA guarantees no transient failures during propagation. See NemoClaw #XXXX.'
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Search for 'WORKAROUND' or 'workaround' in compatible-endpoint-smoke.ts — no matches.
  • Missing regression test: N/A — documentation item. The new tests 'retries a transient non-JSON gateway response' and 'fails after the bounded transient retry budget' simulate the invalid state and verify bounded retry behavior.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Search for 'WORKAROUND' or 'workaround' in compatible-endpoint-smoke.ts — no matches.
  • Evidence: PR body: 'short provider/proxy propagation windows and empty hosted responses.' Runtime evidence: CI shows HTML 504 responses. No code comment marks as workaround.

PRA-6 Resolve/justify — Fragile HTML status extraction — regex only matches <title>/<h1> with 3-digit code

  • Location: src/lib/onboard/compatible-endpoint-smoke.ts:330
  • Category: correctness
  • Problem: The non-JSON response handler extracts HTTP status from HTML title/h1 tags via regex: r"<(?:title|h1)>\s*([1-5][0-9][0-9])\b". This assumes gateway returns HTML with standard title/h1 containing status code. If gateway returns plain text, JSON error, or different HTML structure, status_match will be None and retryable_gateway_error will be false, causing non-retry on potentially transient 5xx.
  • Impact: Transient 5xx errors returned as plain text or non-standard HTML will not be retried, causing false onboarding failures during gateway instability.
  • Recommended action: Also check for 5xx in response body more broadly (e.g., regex for '5[0-9]{2}' anywhere in first 1000 chars), or accept that only HTML gateway errors with standard markup are retryable. Document the assumption.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Lines 330-335: regex only matches <title> or <h1> with 3-digit status.
  • Missing regression test: Add test: non-JSON response without HTML title/h1 but with 503 in body — verify retry behavior.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Lines 330-335: regex only matches <title> or <h1> with 3-digit status.
  • Evidence: Python check_response at lines 325-340 uses re.search(r"<(?:title|h1)>\\s*([1-5][0-9][0-9])\b", body, re.IGNORECASE).

PRA-7 Resolve/justify — Monolith growth: compatible-endpoint-smoke.ts grew by 61 lines (351→412), exceeds 20-line threshold

  • Location: src/lib/onboard/compatible-endpoint-smoke.ts:1
  • Category: architecture
  • Problem: File grew from 351 to 412 lines (+61). Retry loop logic (constants, loop structure, attempt accounting) should be extracted to a separate module or helper function. Keep generated script focused on payload/response handling.
  • Impact: Single file now handles config validation, payload generation, retry loop, response parsing, and script encoding. Harder to test, review, and maintain. Retry logic not reusable.
  • Recommended action: Extract retry loop logic (constants, loop structure, attempt accounting) to a separate module or helper function. Keep generated script focused on payload/response handling.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: File is 412 lines, base was 351. Retry loop spans lines 354-382.
  • Missing regression test: N/A — refactoring item.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: File is 412 lines, base was 351. Retry loop spans lines 354-382.
  • Evidence: Drift context shows delta 61, severity blocker for monolith growth.

PRA-8 Resolve/justify — Magic number COMPATIBLE_ENDPOINT_SMOKE_COMMAND_OVERHEAD_SECONDS = 30 undocumented

  • Location: src/lib/onboard/compatible-endpoint-smoke.ts:38
  • Category: correctness
  • Problem: No comment explains the 30s overhead breakdown (base64 decode, Python script write, temp file exec, shell startup) or derives it empirically.
  • Impact: If actual overhead differs, host timeout (220s) may be too short or too long. Operators cannot tune confidently.
  • Recommended action: Add comment explaining the 30s overhead breakdown or derive empirically.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Line 38: const COMPATIBLE_ENDPOINT_SMOKE_COMMAND_OVERHEAD_SECONDS = 30; — no comment.
  • Missing regression test: N/A — documentation item.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Line 38: const COMPATIBLE_ENDPOINT_SMOKE_COMMAND_OVERHEAD_SECONDS = 30; — no comment.
  • Evidence: Timeout calculation at lines 39-43 uses this constant.

PRA-9 Resolve/justify — Fixed 5s retry delay in smoke script vs E2E exponential backoff (5s, 10s, 15s)

  • Location: src/lib/onboard/compatible-endpoint-smoke.ts:37
  • Category: correctness
  • Problem: SMOKE_RETRY_DELAY_SECONDS=5 fixed in smoke script, but E2E Phase 5 uses sleep $((5 * attempt)) for exponential backoff (5, 10, 15). Inconsistent backoff strategy for similar transient failure scenarios.
  • Impact: Different backoff behaviors for same failure type (gateway transient errors) — smoke script hammers gateway every 5s, E2E backs off. May cause thundering herd or unnecessary delay.
  • Recommended action: Consider exponential backoff in smoke script: sleep $((SMOKE_RETRY_DELAY_SECONDS * attempt)) or document that operators should override retryDelaySeconds for production gateways.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Smoke script line with SMOKE_RETRY_DELAY_SECONDS=5; E2E script line with sleep $((5 * attempt)).
  • Missing regression test: N/A — consistency item.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Smoke script line with SMOKE_RETRY_DELAY_SECONDS=5; E2E script line with sleep $((5 * attempt)).
  • Evidence: Constant at line 37; E2E test-hermes-e2e.sh line ~560.

💡 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.

PRA-10 Improvement — E2E retry hardcodes max_tokens=1024/256 instead of using named constants matching smoke script

  • Location: test/e2e/test-hermes-e2e.sh:620
  • Category: correctness
  • Problem: E2E Phase 5 Direct API test retry loop hardcodes max_tokens=1024/256 instead of using named constants matching smoke script defaults (INITIAL_MAX_TOKENS=256, RETRY_MAX_TOKENS=1024).
  • Impact: If smoke script defaults change, E2E test diverges silently. Duplication increases maintenance burden.
  • Suggested action: Extract INITIAL_MAX_TOKENS=256 and RETRY_MAX_TOKENS=1024 as variables at top of Phase 5 section, mirroring smoke script defaults.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Phase 5 Direct API test loop hardcodes 256 and 1024.
  • Missing regression test: N/A — consistency item.
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: E2E lines 560-580 hardcode max_tokens values; smoke script uses INITIAL_MAX_TOKENS/RETRY_MAX_TOKENS.

PRA-11 Improvement — nonNegativeInt helper simple enough to inline (used once)

  • Location: src/lib/onboard/compatible-endpoint-smoke.ts:55
  • Category: scope
  • Problem: nonNegativeInt helper is simple enough to inline (used once for retryDelaySeconds). If kept, add JSDoc.
  • Impact: Minor abstraction overhead for single-use helper. Adds cognitive load without reuse benefit.
  • Suggested action: Inline the helper at call site: const retryDelaySeconds = Number.isFinite(options.retryDelaySeconds) ? Math.max(0, Math.floor(options.retryDelaySeconds)) : COMPATIBLE_ENDPOINT_SMOKE_RETRY_DELAY_SECONDS;
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: nonNegativeInt at line 55, used once at line 60.
  • Missing regression test: N/A — simplification item.
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: Function defined lines 55-58, called at line 60.

PRA-12 Improvement — Missing negative test for non-retry on non-transient curl exit codes

  • Location: src/lib/onboard/compatible-endpoint-smoke.test.ts:1
  • Category: tests
  • Problem: No test verifies that non-transient curl exit codes (e.g., exit 3 malformed URL) do NOT trigger retry. Loop should exit immediately.
  • Impact: If over-broad retry (PRA-3) is fixed, this test would verify the fix. Without it, regression risk.
  • Suggested action: Add test: 'does not retry on non-transient curl exit (e.g., exit 3 malformed URL)' — verify loop exits immediately on non-transient code. Requires fake curl helper that can simulate exit codes.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: No test for non-transient curl exit codes in test file.
  • Missing regression test: Add test: 'does not retry on non-transient curl exit (e.g., exit 3 malformed URL)'.
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: Test file has 11 tests; none simulate curl exit codes.
Simplification opportunities: 2 possible cuts, net -44 lines possible

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

  • PRA-7 shrink (src/lib/onboard/compatible-endpoint-smoke.ts:1): Retry loop constants (lines 33-39), while loop structure (lines 354-382), attempt accounting logic
    • Replacement: Import from new module: import { buildRetryLoopScript, COMPATIBLE_ENDPOINT_SMOKE_DEFAULTS } from './smoke-retry-loop'
    • Net: -40 lines
    • Safety boundary: Must preserve shellQuote usage, set -eu, temp file cleanup via trap, and exact retry semantics (3 attempts, 5s delay, exit codes 2/3 retryable)
  • PRA-11 delete (src/lib/onboard/compatible-endpoint-smoke.ts:55): function nonNegativeInt(value: number | undefined, fallback: number): number { if (!Number.isFinite(value)) return fallback; const rounded = Math.floor(Number(value)); return rounded >= 0 ? rounded : fallback; }
    • Replacement: const retryDelaySeconds = Number.isFinite(options.retryDelaySeconds) ? Math.max(0, Math.floor(options.retryDelaySeconds)) : COMPATIBLE_ENDPOINT_SMOKE_RETRY_DELAY_SECONDS;
    • Net: -4 lines
    • Safety boundary: Must preserve: accepts undefined, clamps negative to 0, floors non-integers, uses fallback default
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 — Add test simulating curl exit 28 (timeout) on first attempt, valid JSON on second — verify retry and success. Runtime/sandbox/infrastructure paths need behavioral runtime validation: src/lib/onboard/compatible-endpoint-smoke.ts. The retry loop executes inside the sandbox via shell script; unit tests mock the script execution but don't verify actual curl exit code handling, stderr redaction, or HTML parsing fragility under real gateway conditions.
  • PRA-T2 Runtime validation — Add test simulating curl exit 7 (connection refused) on first attempt, valid JSON on second — verify retry and success. Runtime/sandbox/infrastructure paths need behavioral runtime validation: src/lib/onboard/compatible-endpoint-smoke.ts. The retry loop executes inside the sandbox via shell script; unit tests mock the script execution but don't verify actual curl exit code handling, stderr redaction, or HTML parsing fragility under real gateway conditions.
  • PRA-T3 Runtime validation — Add test simulating curl exit 6 (DNS failure) on first attempt, valid JSON on second — verify retry and success. Runtime/sandbox/infrastructure paths need behavioral runtime validation: src/lib/onboard/compatible-endpoint-smoke.ts. The retry loop executes inside the sandbox via shell script; unit tests mock the script execution but don't verify actual curl exit code handling, stderr redaction, or HTML parsing fragility under real gateway conditions.
  • PRA-T4 Runtime validation — Add test simulating curl exit 3 (malformed URL) — verify NO retry, immediate failure. Runtime/sandbox/infrastructure paths need behavioral runtime validation: src/lib/onboard/compatible-endpoint-smoke.ts. The retry loop executes inside the sandbox via shell script; unit tests mock the script execution but don't verify actual curl exit code handling, stderr redaction, or HTML parsing fragility under real gateway conditions.
  • PRA-T5 Runtime validation — Add test with curl stderr containing 'Authorization: Bearer secret' — verify redacted/suppressed in output. Runtime/sandbox/infrastructure paths need behavioral runtime validation: src/lib/onboard/compatible-endpoint-smoke.ts. The retry loop executes inside the sandbox via shell script; unit tests mock the script execution but don't verify actual curl exit code handling, stderr redaction, or HTML parsing fragility under real gateway conditions.
  • PRA-T6 Missing negative test for non-retry on non-transient curl exit codes — Add test: 'does not retry on non-transient curl exit (e.g., exit 3 malformed URL)' — verify loop exits immediately on non-transient code. Requires fake curl helper that can simulate exit codes.
  • PRA-T7 Acceptance clause — including curl failures and non-JSON gateway responses — add test evidence or identify existing coverage. Non-JSON gateway responses tested (504 HTML retry + success, 3×504 exhaust). Curl failures (exit 28/7/6) NOT TESTED — fake curl helper only controls stdout body
  • PRA-T8 Retry loop in buildCompatibleEndpointSandboxSmokeScript (lines 354-382) — Tests 'retries a transient non-JSON gateway response' and 'fails after bounded transient retry budget' cover gateway-level transients. MISSING: curl-level transient tests (exit 28/7/6) — see PRA-2 blocker. PR body: 'short provider/proxy propagation windows and empty hosted responses.' CI shows HTML 504 responses. No code comment marks as workaround.
Since last review details

Current findings, using the urgency labels above:

PRA-1 Resolve/justify — Source-of-truth review needed: Retry loop in buildCompatibleEndpointSandboxSmokeScript (lines 354-382)

  • 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: Tests 'retries a transient non-JSON gateway response' and 'fails after bounded transient retry budget' cover gateway-level transients. MISSING: curl-level transient tests (exit 28/7/6) — see PRA-2 blocker
  • 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: PR body: 'short provider/proxy propagation windows and empty hosted responses.' CI shows HTML 504 responses. No code comment marks as workaround.

PRA-2 Required — Missing test for curl-level transient failures despite PR claiming 'including curl failures'

  • Location: src/lib/onboard/compatible-endpoint-smoke.test.ts:1
  • Category: acceptance
  • Problem: PR description and commit messages state retries cover 'curl failures and non-JSON gateway responses.' Implementation retries on ALL non-zero curl exit codes, but the fake curl helper (writeFakeCurl) only controls stdout body via ${bodyForCall}, not exit codes. No test verifies that curl timeout (exit 28), connection refused (exit 7), or DNS failure (exit 6) actually trigger a retry and succeed.
  • Impact: The claimed 'curl failures' retry behavior is untested. If the fake helper or implementation has a bug handling curl exit codes, transient network failures would fail onboarding silently without retry.
  • Required action: Add test cases simulating curl exit 28 (timeout), exit 7 (connection refused), exit 6 (DNS failure) on first attempt, then valid JSON on second. Modify writeFakeCurl or add a variant that can simulate specific exit codes. Verify loop retries and succeeds.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Search for 'exit 28' or 'timeout' or 'connection refused' or 'DNS' in compatible-endpoint-smoke.test.ts — no matches. The writeFakeCurl helper at line 47 only controls stdout body, not exit codes.
  • Missing regression test: Add test: 'retries on curl timeout (exit 28) then succeeds'. Add test: 'retries on curl connection refused (exit 7) then succeeds'. Add test: 'retries on curl DNS failure (exit 6) then succeeds'.
  • Done when: The required change is committed and verification passes: Search for 'exit 28' or 'timeout' or 'connection refused' or 'DNS' in compatible-endpoint-smoke.test.ts — no matches. The writeFakeCurl helper at line 47 only controls stdout body, not exit codes.
  • Evidence: PR body: 'Retry the production compatible-endpoint sandbox smoke up to three times with bounded backoff, including curl failures and non-JSON gateway responses.' Implementation lines 364 retries on any non-zero status. Test file has no curl exit code simulation.

PRA-3 Resolve/justify — curl stderr printed unredacted — may leak Authorization headers on 401/403

  • Location: src/lib/onboard/compatible-endpoint-smoke.ts:304
  • Category: security
  • Problem: The run_smoke_request function prints curl stderr directly with 'cat "$error_file" >&2'. If curl includes credentials (e.g., Authorization headers) in error output for failed requests, they would flow through sandbox stderr before host-side redaction.
  • Impact: If inference.local returns 401/403, curl may write 'Authorization: Bearer <token>' to stderr. This prints directly to sandbox stderr, captured by host after execution, but redact() only runs on combined stdout+stderr after the fact. If sandbox logs are persisted or forwarded before redaction, credentials leak.
  • Recommended action: Redact sensitive patterns (Authorization: Bearer, apiKey, etc.) from curl stderr before printing, or suppress curl stderr entirely for failed requests since the Python handler already logs structured diagnostics (byte count, HTTP status).
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Lines 304-310: run_smoke_request prints raw curl stderr via 'cat "$error_file" >&2'.
  • Missing regression test: Add test verifying curl stderr with Authorization header does not leak to output.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Lines 304-310: run_smoke_request prints raw curl stderr via 'cat "$error_file" >&2'.
  • Evidence: run_smoke_request at lines 304-310 prints raw curl stderr. Test at line 187 verifies HTML body secret not leaked, but curl stderr path untested.

PRA-4 Resolve/justify — Retry loop retries on ALL non-zero curl exit codes including non-transient ones

  • Location: src/lib/onboard/compatible-endpoint-smoke.ts:364
  • Category: correctness
  • Problem: The retry loop retries on any non-zero status from run_smoke_request. Curl exit codes 1 (unsupported protocol), 2 (init failed), 3 (malformed URL), 4 (feature unsupported), 5 (proxy), 8 (weird reply) are non-transient but trigger retry. Only 6 (DNS), 7 (connection), 28 (timeout), 52 (empty reply), 56 (recv failure) are transient. Non-transient failures (e.g., malformed inference URL, unsupported protocol) will consume retry budget and delay final failure, masking configuration errors.
  • Impact: Configuration errors like malformed URLs or unsupported protocols will waste retry budget (up to 3 attempts × 5s delay = 15s) before failing, delaying feedback and masking root cause.
  • Recommended action: Option A: Map known transient curl exit codes (6, 7, 28, 52, 56) to a retryable code in run_smoke_request, keep loop simple. Option B: Check for transient codes explicitly in loop condition. Option A is cleaner.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect run_smoke_request function at lines 298-310: it returns raw curl exit code. Loop at lines 354-382 retries on any status != 0 when request_failed=1.
  • Missing regression test: Add test: 'does not retry on non-transient curl exit (e.g., exit 3 malformed URL)' — verify loop exits immediately on non-transient code.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect run_smoke_request function at lines 298-310: it returns raw curl exit code. Loop at lines 354-382 retries on any status != 0 when request_failed=1.
  • Evidence: run_smoke_request returns raw curl exit code. Loop condition at line 364: 'if [ "$status" -eq 0 ]' then check_response, else if request_failed=1 retry on any non-zero.

PRA-5 Resolve/justify — Retry loop is undocumented workaround for external gateway instability

  • Location: src/lib/onboard/compatible-endpoint-smoke.ts:354
  • Category: architecture
  • Problem: The retry loop (lines 354-382) handles transient gateway failures but has no comment marking it as a workaround with removal condition. Source-of-truth review needed.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear. Future maintainers may not know this is temporary infrastructure compensation.
  • Recommended action: Add a comment in buildCompatibleEndpointSandboxSmokeScript or at the top of the generated shell script noting: 'WORKAROUND: Retries transient gateway failures (504, timeouts, curl network errors). Remove when gateway SLA guarantees no transient failures during propagation. See NemoClaw #XXXX.'
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Search for 'WORKAROUND' or 'workaround' in compatible-endpoint-smoke.ts — no matches.
  • Missing regression test: N/A — documentation item. The new tests 'retries a transient non-JSON gateway response' and 'fails after the bounded transient retry budget' simulate the invalid state and verify bounded retry behavior.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Search for 'WORKAROUND' or 'workaround' in compatible-endpoint-smoke.ts — no matches.
  • Evidence: PR body: 'short provider/proxy propagation windows and empty hosted responses.' Runtime evidence: CI shows HTML 504 responses. No code comment marks as workaround.

PRA-6 Resolve/justify — Fragile HTML status extraction — regex only matches <title>/<h1> with 3-digit code

  • Location: src/lib/onboard/compatible-endpoint-smoke.ts:330
  • Category: correctness
  • Problem: The non-JSON response handler extracts HTTP status from HTML title/h1 tags via regex: r"<(?:title|h1)>\s*([1-5][0-9][0-9])\b". This assumes gateway returns HTML with standard title/h1 containing status code. If gateway returns plain text, JSON error, or different HTML structure, status_match will be None and retryable_gateway_error will be false, causing non-retry on potentially transient 5xx.
  • Impact: Transient 5xx errors returned as plain text or non-standard HTML will not be retried, causing false onboarding failures during gateway instability.
  • Recommended action: Also check for 5xx in response body more broadly (e.g., regex for '5[0-9]{2}' anywhere in first 1000 chars), or accept that only HTML gateway errors with standard markup are retryable. Document the assumption.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Lines 330-335: regex only matches <title> or <h1> with 3-digit status.
  • Missing regression test: Add test: non-JSON response without HTML title/h1 but with 503 in body — verify retry behavior.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Lines 330-335: regex only matches <title> or <h1> with 3-digit status.
  • Evidence: Python check_response at lines 325-340 uses re.search(r"<(?:title|h1)>\\s*([1-5][0-9][0-9])\b", body, re.IGNORECASE).

PRA-7 Resolve/justify — Monolith growth: compatible-endpoint-smoke.ts grew by 61 lines (351→412), exceeds 20-line threshold

  • Location: src/lib/onboard/compatible-endpoint-smoke.ts:1
  • Category: architecture
  • Problem: File grew from 351 to 412 lines (+61). Retry loop logic (constants, loop structure, attempt accounting) should be extracted to a separate module or helper function. Keep generated script focused on payload/response handling.
  • Impact: Single file now handles config validation, payload generation, retry loop, response parsing, and script encoding. Harder to test, review, and maintain. Retry logic not reusable.
  • Recommended action: Extract retry loop logic (constants, loop structure, attempt accounting) to a separate module or helper function. Keep generated script focused on payload/response handling.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: File is 412 lines, base was 351. Retry loop spans lines 354-382.
  • Missing regression test: N/A — refactoring item.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: File is 412 lines, base was 351. Retry loop spans lines 354-382.
  • Evidence: Drift context shows delta 61, severity blocker for monolith growth.

PRA-8 Resolve/justify — Magic number COMPATIBLE_ENDPOINT_SMOKE_COMMAND_OVERHEAD_SECONDS = 30 undocumented

  • Location: src/lib/onboard/compatible-endpoint-smoke.ts:38
  • Category: correctness
  • Problem: No comment explains the 30s overhead breakdown (base64 decode, Python script write, temp file exec, shell startup) or derives it empirically.
  • Impact: If actual overhead differs, host timeout (220s) may be too short or too long. Operators cannot tune confidently.
  • Recommended action: Add comment explaining the 30s overhead breakdown or derive empirically.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Line 38: const COMPATIBLE_ENDPOINT_SMOKE_COMMAND_OVERHEAD_SECONDS = 30; — no comment.
  • Missing regression test: N/A — documentation item.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Line 38: const COMPATIBLE_ENDPOINT_SMOKE_COMMAND_OVERHEAD_SECONDS = 30; — no comment.
  • Evidence: Timeout calculation at lines 39-43 uses this constant.

PRA-9 Resolve/justify — Fixed 5s retry delay in smoke script vs E2E exponential backoff (5s, 10s, 15s)

  • Location: src/lib/onboard/compatible-endpoint-smoke.ts:37
  • Category: correctness
  • Problem: SMOKE_RETRY_DELAY_SECONDS=5 fixed in smoke script, but E2E Phase 5 uses sleep $((5 * attempt)) for exponential backoff (5, 10, 15). Inconsistent backoff strategy for similar transient failure scenarios.
  • Impact: Different backoff behaviors for same failure type (gateway transient errors) — smoke script hammers gateway every 5s, E2E backs off. May cause thundering herd or unnecessary delay.
  • Recommended action: Consider exponential backoff in smoke script: sleep $((SMOKE_RETRY_DELAY_SECONDS * attempt)) or document that operators should override retryDelaySeconds for production gateways.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Smoke script line with SMOKE_RETRY_DELAY_SECONDS=5; E2E script line with sleep $((5 * attempt)).
  • Missing regression test: N/A — consistency item.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Smoke script line with SMOKE_RETRY_DELAY_SECONDS=5; E2E script line with sleep $((5 * attempt)).
  • Evidence: Constant at line 37; E2E test-hermes-e2e.sh line ~560.

PRA-10 Improvement — E2E retry hardcodes max_tokens=1024/256 instead of using named constants matching smoke script

  • Location: test/e2e/test-hermes-e2e.sh:620
  • Category: correctness
  • Problem: E2E Phase 5 Direct API test retry loop hardcodes max_tokens=1024/256 instead of using named constants matching smoke script defaults (INITIAL_MAX_TOKENS=256, RETRY_MAX_TOKENS=1024).
  • Impact: If smoke script defaults change, E2E test diverges silently. Duplication increases maintenance burden.
  • Suggested action: Extract INITIAL_MAX_TOKENS=256 and RETRY_MAX_TOKENS=1024 as variables at top of Phase 5 section, mirroring smoke script defaults.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Phase 5 Direct API test loop hardcodes 256 and 1024.
  • Missing regression test: N/A — consistency item.
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: E2E lines 560-580 hardcode max_tokens values; smoke script uses INITIAL_MAX_TOKENS/RETRY_MAX_TOKENS.

PRA-11 Improvement — nonNegativeInt helper simple enough to inline (used once)

  • Location: src/lib/onboard/compatible-endpoint-smoke.ts:55
  • Category: scope
  • Problem: nonNegativeInt helper is simple enough to inline (used once for retryDelaySeconds). If kept, add JSDoc.
  • Impact: Minor abstraction overhead for single-use helper. Adds cognitive load without reuse benefit.
  • Suggested action: Inline the helper at call site: const retryDelaySeconds = Number.isFinite(options.retryDelaySeconds) ? Math.max(0, Math.floor(options.retryDelaySeconds)) : COMPATIBLE_ENDPOINT_SMOKE_RETRY_DELAY_SECONDS;
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: nonNegativeInt at line 55, used once at line 60.
  • Missing regression test: N/A — simplification item.
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: Function defined lines 55-58, called at line 60.

PRA-12 Improvement — Missing negative test for non-retry on non-transient curl exit codes

  • Location: src/lib/onboard/compatible-endpoint-smoke.test.ts:1
  • Category: tests
  • Problem: No test verifies that non-transient curl exit codes (e.g., exit 3 malformed URL) do NOT trigger retry. Loop should exit immediately.
  • Impact: If over-broad retry (PRA-3) is fixed, this test would verify the fix. Without it, regression risk.
  • Suggested action: Add test: 'does not retry on non-transient curl exit (e.g., exit 3 malformed URL)' — verify loop exits immediately on non-transient code. Requires fake curl helper that can simulate exit codes.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: No test for non-transient curl exit codes in test file.
  • Missing regression test: Add test: 'does not retry on non-transient curl exit (e.g., exit 3 malformed URL)'.
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: Test file has 11 tests; none simulate curl exit codes.

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 28, 2026

Copy link
Copy Markdown
Collaborator Author

Advisor disposition for commit 4c5abf1:

  • PRA-2 fixed: the parent sandbox-exec timeout is now derived from the same three-attempt, 60-second request, and 5-second backoff constants. Its 220-second budget includes 30 seconds of command overhead. A unit test captures the production runOpenshell call and asserts timeout=220000.
  • PRA-3 fixed: the generated sandbox script no longer prints any non-JSON response body. It reports only parse failure, response byte length, and a parsed HTML status such as 504. The bounded-exhaustion test includes Authorization: Bearer test-secret and asserts the secret is absent from stderr.
  • PRA-1 source boundary: this is the production compatible-endpoint validation boundary, not a test-only workaround. The invalid state is a short provider/proxy propagation window after sandbox reuse; the upstream compatible endpoint is allowed to return transient 5xx responses, and the correct source fix is bounded retry at the authenticated validation boundary. Configuration/provider/credential validation remains strict, and three failed attempts still terminate onboarding. The retry can be removed only if the upstream route contract guarantees transient-free readiness.
  • Tests: nine behavioral unit tests pass, including 504 recovery, bounded 504 exhaustion, secret suppression, reasoning-budget retry, and parent timeout coverage. Biome, managed ShellCheck, Bash syntax, CLI typecheck, normal commit/push hooks, and secret scanning pass.

Current-head token-rotation and Hermes runtime validations are being run sequentially to avoid hosted-service contention.

@github-actions

Copy link
Copy Markdown
Contributor

Selective E2E Results — ✅ All requested jobs passed

Run: 28326589749
Target ref: 30ca8fc34e6c99a2c98f98fc088349a816081ac2
Workflow ref: main
Requested jobs: messaging-compatible-endpoint-e2e
Summary: 1 passed, 0 failed, 0 cancelled, 0 skipped

Job Result
messaging-compatible-endpoint-e2e ✅ success

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ Duplicate comments (1)
src/lib/onboard/compatible-endpoint-smoke.ts (1)

364-381: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Terminal validation failures are still retried with backoff.

check_response exits 1 for transient non-JSON gateway bodies and for deterministic JSON-shape/content failures (Lines 341, 347), and the loop retries any non-zero status until the budget is exhausted. A permanently bad-but-valid JSON response therefore issues extra smoke requests and sleeps on backoff instead of failing fast. Consider a distinct retryable code for the transient non-JSON case and exiting immediately for non-retryable validation failures.

🤖 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/onboard/compatible-endpoint-smoke.ts` around lines 364 - 381, The
smoke loop in compatible-endpoint-smoke.ts treats every non-zero result from
check_response as retryable, so deterministic JSON validation failures get
retried with backoff. Update check_response to distinguish transient non-JSON
gateway bodies from permanent JSON-shape/content failures using a distinct exit
status, then adjust the surrounding retry logic in the run_smoke_request/attempt
loop to retry only the transient case and fail fast on non-retryable validation
errors.
🤖 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.

Duplicate comments:
In `@src/lib/onboard/compatible-endpoint-smoke.ts`:
- Around line 364-381: The smoke loop in compatible-endpoint-smoke.ts treats
every non-zero result from check_response as retryable, so deterministic JSON
validation failures get retried with backoff. Update check_response to
distinguish transient non-JSON gateway bodies from permanent JSON-shape/content
failures using a distinct exit status, then adjust the surrounding retry logic
in the run_smoke_request/attempt loop to retry only the transient case and fail
fast on non-retryable validation errors.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: f0e8ede4-fe57-4849-9f2d-246c2ebeaed2

📥 Commits

Reviewing files that changed from the base of the PR and between 30ca8fc and 4c5abf1.

📒 Files selected for processing (2)
  • src/lib/onboard/compatible-endpoint-smoke.test.ts
  • src/lib/onboard/compatible-endpoint-smoke.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/lib/onboard/compatible-endpoint-smoke.test.ts

@github-actions

Copy link
Copy Markdown
Contributor

Selective E2E Results — ⚠️ Run cancelled — no signal

Run: 28326802190
Target ref: 4c5abf1bbfb4cb8c94f10dc6238196c68fc96b58
Workflow ref: main
Requested jobs: messaging-compatible-endpoint-e2e
Summary: 0 passed, 0 failed, 1 cancelled, 0 skipped

Job Result
messaging-compatible-endpoint-e2e ⚠️ cancelled

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 `@test/e2e/test-hermes-e2e.sh`:
- Around line 632-644: The live API probe in the test script is matching on any
parsed text containing PONG, which can let non-exact responses slip through.
Update the checks around parse_chat_content and the final pass in the direct API
loop to compare the normalized api_content against PONG exactly, so only an
exact one-word response breaks and passes the test.
🪄 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: 65c97f27-ff2d-4a08-8f9e-41661eb47f9d

📥 Commits

Reviewing files that changed from the base of the PR and between 4c5abf1 and cb65f81.

📒 Files selected for processing (3)
  • src/lib/onboard/compatible-endpoint-smoke.test.ts
  • src/lib/onboard/compatible-endpoint-smoke.ts
  • test/e2e/test-hermes-e2e.sh

Comment on lines +632 to +644
api_content="$(printf '%s' "$api_response" | parse_chat_content 2>/dev/null || true)"
api_failure_summary="response without PONG (HTTP ${api_http_status: -3}, chars=${#api_response})"
if grep -qi "PONG" <<<"$api_content"; then
pass "[LIVE] Direct API: model responded with PONG"
else
fail "[LIVE] Direct API: expected PONG, got: ${api_content:0:200}"
break
fi
if [ "$attempt" -lt 3 ]; then
info "[LIVE] Direct API attempt ${attempt}/3 ${api_failure_summary}; retrying..."
sleep $((5 * attempt))
fi
done

if grep -qi "PONG" <<<"$api_content"; then
pass "[LIVE] Direct API: model responded with PONG"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Require an exact PONG match here.

Lines 634 and 643 currently pass on any parsed text that merely contains PONG, so outputs like NOT PONG or extra explanatory text can satisfy the probe without proving the advertised one-word response. Compare the normalized parsed content to PONG exactly before breaking/passing.

As per path instructions, tests should “prefer observable outcomes through the public boundary” and flag “conditionals that make a test pass without exercising its claim.”

Suggested change
-  if grep -qi "PONG" <<<"$api_content"; then
+  if printf '%s\n' "$api_content" | grep -Eqix 'PONG'; then
     break
   fi
@@
-if grep -qi "PONG" <<<"$api_content"; then
+if printf '%s\n' "$api_content" | grep -Eqix 'PONG'; then
   pass "[LIVE] Direct API: model responded with PONG"
 else
   fail "[LIVE] Direct API: expected PONG after 3 attempts; ${api_failure_summary}"
 fi
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
api_content="$(printf '%s' "$api_response" | parse_chat_content 2>/dev/null || true)"
api_failure_summary="response without PONG (HTTP ${api_http_status: -3}, chars=${#api_response})"
if grep -qi "PONG" <<<"$api_content"; then
pass "[LIVE] Direct API: model responded with PONG"
else
fail "[LIVE] Direct API: expected PONG, got: ${api_content:0:200}"
break
fi
if [ "$attempt" -lt 3 ]; then
info "[LIVE] Direct API attempt ${attempt}/3 ${api_failure_summary}; retrying..."
sleep $((5 * attempt))
fi
done
if grep -qi "PONG" <<<"$api_content"; then
pass "[LIVE] Direct API: model responded with PONG"
api_content="$(printf '%s' "$api_response" | parse_chat_content 2>/dev/null || true)"
api_failure_summary="response without PONG (HTTP ${api_http_status: -3}, chars=${`#api_response`})"
if printf '%s\n' "$api_content" | grep -Eqix 'PONG'; then
break
fi
if [ "$attempt" -lt 3 ]; then
info "[LIVE] Direct API attempt ${attempt}/3 ${api_failure_summary}; retrying..."
sleep $((5 * attempt))
fi
done
if printf '%s\n' "$api_content" | grep -Eqix 'PONG'; then
pass "[LIVE] Direct API: model responded with PONG"
else
fail "[LIVE] Direct API: expected PONG after 3 attempts; ${api_failure_summary}"
fi
🤖 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 `@test/e2e/test-hermes-e2e.sh` around lines 632 - 644, The live API probe in
the test script is matching on any parsed text containing PONG, which can let
non-exact responses slip through. Update the checks around parse_chat_content
and the final pass in the direct API loop to compare the normalized api_content
against PONG exactly, so only an exact one-word response breaks and passes the
test.

Source: Path instructions

@github-actions

Copy link
Copy Markdown
Contributor

Selective E2E Results — ✅ All requested jobs passed

Run: 28326902209
Target ref: cb65f819621b0d73fe78768e57aced8714032daa
Workflow ref: main
Requested jobs: messaging-compatible-endpoint-e2e
Summary: 1 passed, 0 failed, 0 cancelled, 0 skipped

Job Result
messaging-compatible-endpoint-e2e ✅ success

@cv
cv merged commit 6d81392 into main Jun 28, 2026
46 checks passed
@cv
cv deleted the codex/retry-hosted-smoke branch June 28, 2026 15:33
cv added a commit that referenced this pull request Jun 28, 2026
<!-- markdownlint-disable MD041 -->
## Summary
Tightens the compatible-endpoint smoke retry merged in #5932 so only
explicit transient transport and HTTP signals retry. It also removes
curl stderr/body dependence, derives exponential-backoff timeout
coverage, and adds behavioral tests for transient and permanent curl
exits.

## Changes
- Capture HTTP status directly from curl instead of inferring it from
HTML response bodies.
- Suppress curl stderr and retry only transient exits 6, 7, 28, 52, 55,
and 56.
- Keep permanent curl and JSON validation failures single-shot and
strict.
- Align retry delay with the existing Vitest 5s/10s backoff and derive
the 225-second parent timeout from the shared budget.
- Centralize the generated retry policy and triangular timeout budget in
a focused classifier module.
- Add behavioral coverage for transient curl exits 6, 7, 28, 52, 55, and
56; permanent curl and HTTP 429 failures; hostile model quoting; and
5s/10s backoff.

## Type of Change

- [x] 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
<!-- Check all that apply. For any "covered by existing tests", "not
applicable", or waiver entry, add a brief justification on the same line
or in the Changes section. -->
- [x] Tests added or updated for changed behavior
- [ ] Existing tests cover changed behavior — justification:
- [ ] Tests not applicable — justification:
- [ ] Docs updated for user-facing behavior changes
- [x] Docs not applicable — justification: internal validation retry
classification and diagnostics only; no user-facing configuration
changes.
- [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: arbitrary curl stderr
and upstream response bodies are suppressed; diagnostics retain only
exit code, HTTP status, and response length, and all permanent
validation failures remain strict.
- [ ] Non-success, skipped, or missing CI check accepted by maintainer —
check name, approval link, and follow-up issue:

## Verification
<!-- Check each item you ran and confirmed. Leave unchecked items you
skipped. Doc-only changes do not require npm test unless you ran it. -->
- [x] PR description includes the DCO sign-off declaration and every
commit appears as `Verified` in GitHub
- [x] Git hooks passed during commit and push, or `npx prek run
--from-ref main --to-ref HEAD` passes
- [x] Targeted tests pass for changed behavior
- [ ] Full `npm test` passes (broad runtime changes only)
- [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)
- [ ] 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)

Targeted verification:
- `npx vitest run --project cli
src/lib/onboard/compatible-endpoint-smoke.test.ts
src/lib/onboard/smoke-retry-classifier.test.ts` (36 tests passed)
- `npx biome check src/lib/onboard/compatible-endpoint-smoke.ts
src/lib/onboard/compatible-endpoint-smoke.test.ts`
- `npm run typecheck:cli`
- `git diff --check`
- Normal commit and push hooks passed, including the full CLI suite,
repository checks, and secret scanning.
- Token rotation reused-sandbox runtime:
https://github.com/NVIDIA/NemoClaw/actions/runs/28327364831 (success).
- Hermes dashboard runtime:
https://github.com/NVIDIA/NemoClaw/actions/runs/28328424431 (success).

Post-merge review findings addressed from #5932:
- Curl-exit retry acceptance coverage: transient exits 6, 7, 28, 52, 55,
and 56 recover on attempt two; permanent exit 2 makes exactly one
request.
- Curl stderr security: stderr is suppressed at the external boundary
and only the numeric exit is reported.
- Retry classification: only explicit transient transport codes, HTTP
5xx, and reasoning-budget exhaustion retry; HTTP 429 remains
single-shot.
- HTTP classification: status comes from curl metadata rather than
fragile HTML matching.
- Retry timing: 5s/10s backoff and its 225-second parent budget are
derived from shared constants.

---
<!-- 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: Carlos Villela <cvillela@nvidia.com>


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

* **Bug Fixes**
* Improved compatible-endpoint smoke verification with more
deterministic HTTP-status handling to classify retries vs terminal
failures, including clearer retry timing and faster exits for
non-retryable outcomes.

* **Tests**
  * Expanded smoke-test coverage for retry behavior:
    * Transient HTTP 5xx responses now retry and succeed.
    * HTTP 429 responses no longer retry.
* Added scenarios for multiple transient curl exit codes (retries
succeed) and terminal exit codes (fails immediately).
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Signed-off-by: Carlos Villela <cvillela@nvidia.com>
cv pushed a commit that referenced this pull request Jun 29, 2026
## Summary
Adds the v0.0.69 release notes to the published release-notes page so
users can see the shipped sandbox recovery, Deep Agents Code, Hermes,
inference, policy, and release-validation changes.
The section is based on the v0.0.69 announcement and links each
user-facing theme to the deeper docs pages that already cover the
behavior.

## Changes
- Added a new `v0.0.69` section to `docs/about/release-notes.mdx`.
- Linked release-note themes to lifecycle, backup, troubleshooting, Deep
Agents Code, commands, workspace, messaging, Hermes, inference,
security, monitoring, and network-policy docs.

Source summary:
- #5455 -> `docs/about/release-notes.mdx`: Summarized persistent
workspace and state cleanup during sandbox destroy.
- #5738 -> `docs/about/release-notes.mdx`: Summarized nonzero exit
status preservation for failed hosted endpoint validation.
- #5786 -> `docs/about/release-notes.mdx`: Summarized live sandbox
rediscovery when local registry state is missing.
- #5881 -> `docs/about/release-notes.mdx`: Summarized the
`nemo-deepagents` alias command surface.
- #5594 -> `docs/about/release-notes.mdx`: Summarized the Hermes Agent
2026.6.19 update.
- #5777 -> `docs/about/release-notes.mdx`: Summarized manifest-derived
messaging channel support.
- #5825 -> `docs/about/release-notes.mdx`: Summarized DeepSeek V4 Flash
managed-vLLM defaults for DGX Station.
- #5877 -> `docs/about/release-notes.mdx`: Summarized provider switch
metadata preservation.
- #5932 -> `docs/about/release-notes.mdx`: Summarized transient
inference smoke retry behavior.
- #5934 -> `docs/about/release-notes.mdx`: Summarized constrained
inference smoke retry boundaries.
- #5681 -> `docs/about/release-notes.mdx`: Summarized Shields
config-hash sealing during auto-restore.
- #5682 -> `docs/about/release-notes.mdx`: Summarized sandbox connect
process-limit enforcement.
- #5683 -> `docs/about/release-notes.mdx`: Summarized JSON agent failure
provenance warnings.
- #5711 -> `docs/about/release-notes.mdx`: Summarized sparse-source log
breadcrumbs.
- #5838 -> `docs/about/release-notes.mdx`: Summarized host-authoritative
Shields status.
- #5880 -> `docs/about/release-notes.mdx`: Summarized policy round-trip
documentation updates.
- #5886 -> `docs/about/release-notes.mdx`: Summarized network request
approval-flow documentation updates.

## 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
- [ ] Existing tests cover changed behavior — justification:
- [x] Tests not applicable — justification: doc-only release-notes
prose; no runtime behavior changed.
- [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:

## Verification
- [x] PR description includes the DCO sign-off declaration and every
commit appears as `Verified` in GitHub
- [x] 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)
- [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)
- [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)

`npm run docs` passed with 0 errors and the existing Fern light-mode
accent contrast warning.
`fern check --warnings` reported the same accent-color warning.

---
Signed-off-by: Miyoung Choi <miyoungc@nvidia.com>

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

* **Documentation**
* Added release notes for **v0.0.69**, covering improved sandbox
lifecycle recovery (state preservation across
destroy/recreate/rebuild/recovery/validation failures), clearer Deep
Agents Code terminal/CLI behavior, and safer Hermes messaging/provider
switching with manifest-driven channels.
* Improved inference setup validation guidance, including handling of
local/compatible endpoints and redaction of sensitive validation errors.
* Refreshed release-gate documentation with clearer approval examples
and validation behavior for NVIDIA API keys vs hosted inference keys.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
Hadar301 pushed a commit to Hadar301/NemoClaw-OpenShift that referenced this pull request Jul 12, 2026
<!-- markdownlint-disable MD041 -->
## Summary
Retries transient compatible-endpoint sandbox smoke failures during
onboarding and aligns the retained Hermes direct hosted probe with its
already-retrying Vitest counterpart. This prevents short provider/proxy
propagation windows and empty hosted responses from masking the behavior
under test while retaining a strict, bounded failure budget.

## Changes
- Retry the production compatible-endpoint sandbox smoke up to three
times with bounded backoff, including curl failures and non-JSON gateway
responses.
- Preserve the larger-token retry used for reasoning-only responses and
fail after the configured attempt budget.
- Add executable recovery and exhaustion tests using a fake `504 Gateway
Time-out` response.
- Retry the retained Hermes direct hosted `PONG` probe three times,
matching the live Vitest lane.

## Type of Change

- [x] 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
<!-- Check all that apply. For any "covered by existing tests", "not
applicable", or waiver entry, add a brief justification on the same line
or in the Changes section. -->
- [x] Tests added or updated for changed behavior
- [x] Existing tests cover changed behavior — justification: the live
Hermes Vitest lane already exercises three-attempt direct and sandbox
hosted inference retries.
- [ ] Tests not applicable — justification:
- [ ] Docs updated for user-facing behavior changes
- [x] Docs not applicable — justification: bounded resilience for
existing validation probes; no user-facing configuration or workflow
changes.
- [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: retry scope is limited
to the existing authenticated validation request; provider
configuration, credential binding, redaction, and strict final failure
behavior are unchanged.
- [ ] Non-success, skipped, or missing CI check accepted by maintainer —
check name, approval link, and follow-up issue:

## Verification
<!-- Check each item you ran and confirmed. Leave unchecked items you
skipped. Doc-only changes do not require npm test unless you ran it. -->
- [x] PR description includes the DCO sign-off declaration and every
commit appears as `Verified` in GitHub
- [x] Git hooks passed during commit and push, or `npx prek run
--from-ref main --to-ref HEAD` passes
- [x] Targeted tests pass for changed behavior
- [ ] Full `npm test` passes (broad runtime changes only)
- [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)
- [ ] 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)

Targeted verification:
- `npx vitest run --project cli
src/lib/onboard/compatible-endpoint-smoke.test.ts` (8 tests passed)
- `npx biome check src/lib/onboard/compatible-endpoint-smoke.ts
src/lib/onboard/compatible-endpoint-smoke.test.ts`
- `bash -n test/e2e/test-hermes-e2e.sh`
- `npx prek run shellcheck --files test/e2e/test-hermes-e2e.sh`
- `npm run typecheck:cli`
- `git diff --check`

Runtime evidence on `c4ad10eceb811a8803700237a86978ac93142370`:
- Full Nightly run 28324129704: 66 successful, 4 skipped, and two
failures. Hermes dashboard had one empty direct hosted response while
all dashboard and routed checks passed. Token rotation had one
same-token reuse smoke receive an HTML 504 while all rotation assertions
passed.
- Isolated Hermes dashboard run 28325346077 passed on the same SHA.
- Isolated token rotation run 28325550172 reproduced HTML 504 responses
in same-token reuse phases 5 and 7 while all rebuild/rotation phases
passed, confirming the bounded sandbox-smoke retry is required
independently of full-gate load.

---
<!-- 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: Carlos Villela <cvillela@nvidia.com>


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

* **New Features**
* Added configurable retry behavior for compatible-endpoint smoke checks
(custom attempts and retry delay), including improved shell-script
environment parameters.
* **Bug Fixes**
* Made smoke checks more resilient to transient non-JSON gateway errors
and updated retry decisioning for 5xx conditions and max-token
scenarios.
* Updated sandbox smoke command timeout calculation to match retry and
request settings.
* Improved hosted inference endpoint e2e test retries with clearer “PONG
not found” failure output.
* **Tests**
* Expanded sandbox smoke expectations to validate retry counts,
timeouts, and curl invocation across success and multiple failure modes.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Hadar301 pushed a commit to Hadar301/NemoClaw-OpenShift that referenced this pull request Jul 12, 2026
<!-- markdownlint-disable MD041 -->
## Summary
Tightens the compatible-endpoint smoke retry merged in NVIDIA#5932 so only
explicit transient transport and HTTP signals retry. It also removes
curl stderr/body dependence, derives exponential-backoff timeout
coverage, and adds behavioral tests for transient and permanent curl
exits.

## Changes
- Capture HTTP status directly from curl instead of inferring it from
HTML response bodies.
- Suppress curl stderr and retry only transient exits 6, 7, 28, 52, 55,
and 56.
- Keep permanent curl and JSON validation failures single-shot and
strict.
- Align retry delay with the existing Vitest 5s/10s backoff and derive
the 225-second parent timeout from the shared budget.
- Centralize the generated retry policy and triangular timeout budget in
a focused classifier module.
- Add behavioral coverage for transient curl exits 6, 7, 28, 52, 55, and
56; permanent curl and HTTP 429 failures; hostile model quoting; and
5s/10s backoff.

## Type of Change

- [x] 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
<!-- Check all that apply. For any "covered by existing tests", "not
applicable", or waiver entry, add a brief justification on the same line
or in the Changes section. -->
- [x] Tests added or updated for changed behavior
- [ ] Existing tests cover changed behavior — justification:
- [ ] Tests not applicable — justification:
- [ ] Docs updated for user-facing behavior changes
- [x] Docs not applicable — justification: internal validation retry
classification and diagnostics only; no user-facing configuration
changes.
- [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: arbitrary curl stderr
and upstream response bodies are suppressed; diagnostics retain only
exit code, HTTP status, and response length, and all permanent
validation failures remain strict.
- [ ] Non-success, skipped, or missing CI check accepted by maintainer —
check name, approval link, and follow-up issue:

## Verification
<!-- Check each item you ran and confirmed. Leave unchecked items you
skipped. Doc-only changes do not require npm test unless you ran it. -->
- [x] PR description includes the DCO sign-off declaration and every
commit appears as `Verified` in GitHub
- [x] Git hooks passed during commit and push, or `npx prek run
--from-ref main --to-ref HEAD` passes
- [x] Targeted tests pass for changed behavior
- [ ] Full `npm test` passes (broad runtime changes only)
- [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)
- [ ] 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)

Targeted verification:
- `npx vitest run --project cli
src/lib/onboard/compatible-endpoint-smoke.test.ts
src/lib/onboard/smoke-retry-classifier.test.ts` (36 tests passed)
- `npx biome check src/lib/onboard/compatible-endpoint-smoke.ts
src/lib/onboard/compatible-endpoint-smoke.test.ts`
- `npm run typecheck:cli`
- `git diff --check`
- Normal commit and push hooks passed, including the full CLI suite,
repository checks, and secret scanning.
- Token rotation reused-sandbox runtime:
https://github.com/NVIDIA/NemoClaw/actions/runs/28327364831 (success).
- Hermes dashboard runtime:
https://github.com/NVIDIA/NemoClaw/actions/runs/28328424431 (success).

Post-merge review findings addressed from NVIDIA#5932:
- Curl-exit retry acceptance coverage: transient exits 6, 7, 28, 52, 55,
and 56 recover on attempt two; permanent exit 2 makes exactly one
request.
- Curl stderr security: stderr is suppressed at the external boundary
and only the numeric exit is reported.
- Retry classification: only explicit transient transport codes, HTTP
5xx, and reasoning-budget exhaustion retry; HTTP 429 remains
single-shot.
- HTTP classification: status comes from curl metadata rather than
fragile HTML matching.
- Retry timing: 5s/10s backoff and its 225-second parent budget are
derived from shared constants.

---
<!-- 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: Carlos Villela <cvillela@nvidia.com>


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

* **Bug Fixes**
* Improved compatible-endpoint smoke verification with more
deterministic HTTP-status handling to classify retries vs terminal
failures, including clearer retry timing and faster exits for
non-retryable outcomes.

* **Tests**
  * Expanded smoke-test coverage for retry behavior:
    * Transient HTTP 5xx responses now retry and succeed.
    * HTTP 429 responses no longer retry.
* Added scenarios for multiple transient curl exit codes (retries
succeed) and terminal exit codes (fails immediately).
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Hadar301 pushed a commit to Hadar301/NemoClaw-OpenShift that referenced this pull request Jul 12, 2026
## Summary
Adds the v0.0.69 release notes to the published release-notes page so
users can see the shipped sandbox recovery, Deep Agents Code, Hermes,
inference, policy, and release-validation changes.
The section is based on the v0.0.69 announcement and links each
user-facing theme to the deeper docs pages that already cover the
behavior.

## Changes
- Added a new `v0.0.69` section to `docs/about/release-notes.mdx`.
- Linked release-note themes to lifecycle, backup, troubleshooting, Deep
Agents Code, commands, workspace, messaging, Hermes, inference,
security, monitoring, and network-policy docs.

Source summary:
- NVIDIA#5455 -> `docs/about/release-notes.mdx`: Summarized persistent
workspace and state cleanup during sandbox destroy.
- NVIDIA#5738 -> `docs/about/release-notes.mdx`: Summarized nonzero exit
status preservation for failed hosted endpoint validation.
- NVIDIA#5786 -> `docs/about/release-notes.mdx`: Summarized live sandbox
rediscovery when local registry state is missing.
- NVIDIA#5881 -> `docs/about/release-notes.mdx`: Summarized the
`nemo-deepagents` alias command surface.
- NVIDIA#5594 -> `docs/about/release-notes.mdx`: Summarized the Hermes Agent
2026.6.19 update.
- NVIDIA#5777 -> `docs/about/release-notes.mdx`: Summarized manifest-derived
messaging channel support.
- NVIDIA#5825 -> `docs/about/release-notes.mdx`: Summarized DeepSeek V4 Flash
managed-vLLM defaults for DGX Station.
- NVIDIA#5877 -> `docs/about/release-notes.mdx`: Summarized provider switch
metadata preservation.
- NVIDIA#5932 -> `docs/about/release-notes.mdx`: Summarized transient
inference smoke retry behavior.
- NVIDIA#5934 -> `docs/about/release-notes.mdx`: Summarized constrained
inference smoke retry boundaries.
- NVIDIA#5681 -> `docs/about/release-notes.mdx`: Summarized Shields
config-hash sealing during auto-restore.
- NVIDIA#5682 -> `docs/about/release-notes.mdx`: Summarized sandbox connect
process-limit enforcement.
- NVIDIA#5683 -> `docs/about/release-notes.mdx`: Summarized JSON agent failure
provenance warnings.
- NVIDIA#5711 -> `docs/about/release-notes.mdx`: Summarized sparse-source log
breadcrumbs.
- NVIDIA#5838 -> `docs/about/release-notes.mdx`: Summarized host-authoritative
Shields status.
- NVIDIA#5880 -> `docs/about/release-notes.mdx`: Summarized policy round-trip
documentation updates.
- NVIDIA#5886 -> `docs/about/release-notes.mdx`: Summarized network request
approval-flow documentation updates.

## 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
- [ ] Existing tests cover changed behavior — justification:
- [x] Tests not applicable — justification: doc-only release-notes
prose; no runtime behavior changed.
- [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:

## Verification
- [x] PR description includes the DCO sign-off declaration and every
commit appears as `Verified` in GitHub
- [x] 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)
- [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)
- [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)

`npm run docs` passed with 0 errors and the existing Fern light-mode
accent contrast warning.
`fern check --warnings` reported the same accent-color warning.

---
Signed-off-by: Miyoung Choi <miyoungc@nvidia.com>

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

* **Documentation**
* Added release notes for **v0.0.69**, covering improved sandbox
lifecycle recovery (state preservation across
destroy/recreate/rebuild/recovery/validation failures), clearer Deep
Agents Code terminal/CLI behavior, and safer Hermes messaging/provider
switching with manifest-driven channels.
* Improved inference setup validation guidance, including handling of
local/compatible endpoints and redaction of sensitive validation errors.
* Refreshed release-gate documentation with clearer approval examples
and validation behavior for NVIDIA API keys vs hosted inference keys.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
@wscurran wscurran added area: inference Inference routing, serving, model selection, or outputs area: onboarding Onboarding FSM, provider setup, sandbox launch, or first-run flow labels Aug 1, 2026
@wscurran wscurran added bug-fix PR fixes a bug or regression integration: hermes Hermes integration behavior labels Aug 1, 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 area: onboarding Onboarding FSM, provider setup, sandbox launch, or first-run flow bug-fix PR fixes a bug or regression integration: hermes Hermes integration behavior

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants