fix(advisor): retry transient provider failures by cv · Pull Request #6591 · NVIDIA/NemoClaw

cv · 2026-07-09T18:31:46Z

Summary

Adds model-aware same-session retries for transient PR Review Advisor provider failures, using one retry layer and staggered GPT/Nemotron backoff. The workflow still publishes summaries, comments, and artifacts after an incomplete analysis, then fails an explicit final outcome gate so the missing review cannot appear green.

Changes

Configure four bounded agent-level retries while disabling the provider SDK's overlapping retry layer.
Clear transient attempt errors after a successful retry and preserve the final provider error when retries are exhausted.
Add a final workflow outcome check after summary, comment, and artifact publication, with boundary validation that prevents the failure from being masked.
Add retry and workflow regression tests and document the maintainer-facing behavior.

Type of Change

Code change (feature, bug fix, or refactor)
Code change with doc updates
Doc only (prose changes, no code sample modifications)
Doc only (includes code sample changes)

Quality Gates

Tests added or updated for changed behavior
Existing tests cover changed behavior — justification:
Tests not applicable — justification:
Docs updated for user-facing behavior changes
Docs not applicable — justification: this changes internal PR Review Advisor behavior; tools/pr-review-advisor/README.md documents the maintainer-facing contract and no public NemoClaw behavior 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:
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
Normal pre-commit, commit-msg, and pre-push hooks passed, or npm run check:diff passed when hooks were skipped or unavailable
Targeted behavior tests pass for the current change set, or tests are marked not applicable above — command/result or justification: npx vitest run test/advisor-session-runner.test.ts test/pr-review-advisor-workflow-boundary.test.ts test/pr-review-advisor.test.ts (3 files, 74 tests passed); npm run typecheck:cli, npm run source-shape:check, and npm run test-size:check passed.
Applicable broad gate passed — npm test for broad runtime/test-harness changes; npm run check for repo-wide validation/coverage changes — command/result: not applicable; the change is isolated to advisor retry and workflow-boundary behavior with focused coverage.
Quality Gates section completed with required justifications or waivers
No secrets, API keys, or credentials committed
npm run docs builds without warnings (doc changes only)
Doc pages follow the style guide (doc changes only)
New doc pages include SPDX header and frontmatter (new pages only)

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

Summary by CodeRabbit

New Features
- Added bounded retry handling for transient advisor provider failures within the same session, improving reliability.
- Added an explicit post-analysis verification step to ensure advisor results complete successfully.
Bug Fixes
- Prevents the workflow from continuing when the analysis outcome is missing, not successful, or when verification is misconfigured.
- Improves reporting when transient retries are exhausted.
Documentation
- Updated the PR Review Advisor workflow documentation to reflect the new transient retry behavior and revised step ordering.

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

coderabbitai · 2026-07-09T18:32:14Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: f9b7ec82-4531-4712-be11-559efc51650d

📥 Commits

Reviewing files that changed from the base of the PR and between 105391b and fd977d7.

📒 Files selected for processing (1)

test/advisor-session-runner.test.ts

🚧 Files skipped from review as they are similar to previous changes (1)

test/advisor-session-runner.test.ts

📝 Walkthrough

Walkthrough

This PR adds same-session retry handling for transient provider failures in advisor sessions, adds a workflow step that fails when analysis does not succeed, and updates boundary validation, tests, and workflow docs to match the new flow.

Changes

Advisor retry handling and workflow verification

Layer / File(s)	Summary
Retry configuration and event handling `tools/advisors/session.mts`	Adds exported `advisorRetrySettings(modelId)`, wires it into `SettingsManager.inMemory`, updates `auto_retry_start`/`auto_retry_end` handling to clear or capture `currentTurnError`, and simplifies turn error throwing to always construct `new Error(settlement.turn.error)`.
Session retry test harness `test/advisor-session-runner.test.ts`	Adds a `retryResponses` queue to mocked state, resets it per test, updates mocked `prompt()` to emit transient errors and retry events, imports `advisorRetrySettings`, and adds tests validating retry configuration and clearing/retaining transient errors.
Workflow verification step `.github/workflows/pr-review-advisor.yaml`	Adds a "Verify advisor analysis outcome" step that fails the job when `steps.analysis.outcome` is not `success`.
Workflow boundary validations `tools/pr-review-advisor/workflow-boundary.mts`	Requires `continue-on-error: true` on the advisor run step and adds validation of the new verification step's `if`, env mapping, failure guard, and step ordering.
Boundary tests and README `test/pr-review-advisor-workflow-boundary.test.ts`, `tools/pr-review-advisor/README.md`	Adds tests for missing-step and continue-on-error errors, updates expected error lists, and updates workflow documentation for the retry layer.

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

Sequence Diagram(s)

sequenceDiagram
  participant Session as Advisor Session
  participant SettingsManager
  participant Provider

  Session->>SettingsManager: inMemory(advisorRetrySettings(modelId))
  Session->>Provider: prompt()
  Provider-->>Session: transient error (HTTP 429)
  Session->>Session: auto_retry_start (clear currentTurnError, log attempt/delay)
  Provider-->>Session: retry response
  alt retry succeeded
    Session->>Session: auto_retry_end (clear currentTurnError)
  else retries exhausted
    Session->>Session: auto_retry_end (captureTurnError finalError)
  end

Suggested labels: area: ci, 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 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly matches the main change: adding retries for transient advisor provider 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/advisor-retry-outcome

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

github-code-quality · 2026-07-09T18:32:29Z

Code Coverage Overview

Languages: TypeScript

TypeScript / code-coverage/plugin

The overall coverage remains at 96%, unchanged from the main branch.

TypeScript / code-coverage/cli

The overall coverage in the codex/advisor-retry-... branch remains at 77%, unchanged from the main branch.

Show a code coverage summary of the most impacted files.

File	`main` ac9e1bc	`codex/advisor-retry-...` fd977d7	+/-
`src/lib/actions...-add-restart.ts`	19%	14%	-5%
`src/lib/actions...ridge-policy.ts`	62%	64%	+2%
`src/lib/actions...estore-phase.ts`	87%	93%	+6%
`src/lib/actions...lution-probe.ts`	88%	94%	+6%
`src/lib/actions...de-preflight.ts`	51%	60%	+9%
`src/lib/actions...e-validation.ts`	81%	90%	+9%
`src/lib/actions...x/mcp-bridge.ts`	35%	44%	+9%
`src/lib/onboard...e-patch-flow.ts`	61%	71%	+10%
`src/lib/actions...confirmation.ts`	69%	81%	+12%
`src/lib/actions...light-guards.ts`	71%	88%	+17%

_{Updated July 09, 2026 18:48 UTC
Code Coverage is in Public Preview. Learn more and provide us with your feedback.}

github-actions · 2026-07-09T18:32:52Z

E2E Advisor Recommendation

Required E2E: None
Optional E2E: None

Workflow run

Full advisor summary

E2E Recommendation Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required E2E

None. No existing NemoClaw E2E job is required. The deterministic risk plan lists no required jobs, and the changed files are confined to PR Review Advisor CI tooling, its trusted-workflow boundary checks, session retry handling, tests, and documentation. They do not affect installer/onboarding, sandbox lifecycle, runtime credentials, network policy, inference routing, deployment, or real assistant user flows.

Optional E2E

None.

New E2E recommendations

None.

github-actions · 2026-07-09T18:32:53Z

E2E Target Recommendation

Required E2E targets: None
Optional E2E targets: None

Workflow run

Full E2E target advisor summary

E2E Target Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required E2E targets

None. No NemoClaw E2E target dispatch is required: the PR changes PR Review Advisor workflow/tooling, documentation, and non-test/e2e unit tests only, with no changes to .github/workflows/e2e.yaml, E2E target registry/runtime support, live E2E files, fixtures, or target-relevant product surfaces.

Optional E2E targets

None.

Relevant changed files

None.

github-actions · 2026-07-09T18:36:13Z

PR Review Advisor (Nemotron Ultra) — Changes requested

Merge posture: Do not merge yet
Primary next action: Resolve or justify PRA-1: Merge conflict risk with open PR #5755 on same workflow file.
Open items: 0 required · 2 warnings · 1 suggestion · 7 test follow-ups
Since last review: 0 prior items resolved · 3 still apply · 0 new items found

Action checklist

Findings index

ID	Severity	Category	Location	Required action
`PRA-1`	Resolve/justify	workflow	`.github/workflows/pr-review-advisor.yaml:1`	Coordinate with PR #5755 author to rebase or resolve conflict before either merges. Verify the final workflow retains both the updated checkout action pin and the new outcome verification step.
`PRA-2`	Resolve/justify	tests	`tools/pr-review-advisor/workflow-boundary.mts:1`	Add risk plan entries for: (a) retry contract invariant — maxRetries=4, provider.maxRetries=0, baseDelayMs model-specific; (b) workflow outcome step invariant — must run after artifact upload, if: always(), continue-on-error=false, reads steps.analysis.outcome.
`PRA-3`	Improvement	tests	`test/advisor-session-runner.test.ts:1`	Add targeted tests: (a) verify retry delay progression matches 6s/12s/24s/48s (GPT) and 9s/18s/36s/72s (Nemotron); (b) simulate two concurrent turns both receiving auto_retry events; (c) inject 503/ECONNRESET errors and verify retry contract; (d) integration-style test that workflow uploads artifacts/posts comment then fails on analysis outcome.

Review findings by urgency: 0 required fixes, 2 items to resolve/justify, 1 in-scope improvement

⚠️ 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 — Merge conflict risk with open PR #5755 on same workflow file

Location: .github/workflows/pr-review-advisor.yaml:1
Category: workflow
Problem: PR chore(deps): bump actions/checkout from 6.0.3 to 7.0.0 #5755 (chore(deps): bump actions/checkout from 6.0.3 to 7.0.0) modifies the same workflow file. If both PRs merge without coordination, the actions/checkout version pin could conflict with the new 'Verify advisor analysis outcome' step and retry logic added in this PR.
Impact: Merge conflict could silently drop the new workflow verification step or revert the checkout version, weakening the trusted-code boundary or breaking the pinned action requirement.
Recommended action: Coordinate with PR chore(deps): bump actions/checkout from 6.0.3 to 7.0.0 #5755 author to rebase or resolve conflict before either merges. Verify the final workflow retains both the updated checkout action pin and the new outcome verification step.
Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
Verification: Check PR chore(deps): bump actions/checkout from 6.0.3 to 7.0.0 #5755 diff against this PR's workflow changes; attempt a test merge to confirm no silent drops.
Missing regression test: Add a workflow boundary test that validates the actions/checkout version pin remains at a full commit SHA after merge with dependency updates.
Done when: The risk is fixed or explicitly justified in the PR. Verification: Check PR chore(deps): bump actions/checkout from 6.0.3 to 7.0.0 #5755 diff against this PR's workflow changes; attempt a test merge to confirm no silent drops.
Evidence: Open PR chore(deps): bump actions/checkout from 6.0.3 to 7.0.0 #5755 listed in openPrOverlaps with sameFiles: ['.github/workflows/pr-review-advisor.yaml'] This PR adds 'Verify advisor analysis outcome' step and retry logic to the same workflow

`PRA-2` Resolve/justify — Risk plan missing regression invariants for new retry/outcome logic

Location: tools/pr-review-advisor/workflow-boundary.mts:1
Category: tests
Problem: The deterministic risk plan (planHash e02bcef1) shows families: [] and requiredJobs: [] for this 6-file change. The PR adds: (1) provider-aware retry layer with 4-attempt exponential backoff that can mask transient failures, and (2) workflow outcome verification step that must run after artifact upload with continue-on-error=false. Neither has declared invariants or required jobs in the risk plan.
Impact: Without declared invariants, future changes could weaken the retry contract (e.g., increase maxRetries, change baseDelayMs) or reorder the outcome verification step before artifact upload without triggering deterministic regression checks.
Recommended action: Add risk plan entries for: (a) retry contract invariant — maxRetries=4, provider.maxRetries=0, baseDelayMs model-specific; (b) workflow outcome step invariant — must run after artifact upload, if: always(), continue-on-error=false, reads steps.analysis.outcome.
Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
Verification: Inspect the checked-in risk plan at the PR head commit; verify families/requiredJobs arrays are empty for this change.
Missing regression test: Add a test that asserts the risk plan for this commit includes invariants covering advisorRetrySettings() contract and workflow outcome step ordering.
Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the checked-in risk plan at the PR head commit; verify families/requiredJobs arrays are empty for this change.
Evidence: riskPlan.families: [] and riskPlan.requiredJobs: [] in scope/risk context New retry logic in session.mts: advisorRetrySettings() with maxRetries=4, provider.maxRetries=0 New workflow step 'Verify advisor analysis outcome' with boundary validation in workflow-boundary.mts

💡 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-3` Improvement — Retry and workflow outcome gate test coverage gaps

Location: test/advisor-session-runner.test.ts:1
Category: tests
Problem: Unit tests cover retry success/exhaustion and workflow boundary validation, but miss: (1) exponential backoff delay progression (2nd/3rd/4th retry delays), (2) concurrent turn retry isolation (multiple turns hitting retries), (3) non-429 transient provider errors (5xx, network timeout), (4) behavioral test that workflow publishes artifacts/comments before outcome gate fails.
Impact: Without these tests, regressions in retry timing, turn isolation, error classification, or workflow publish-before-fail behavior could go undetected until runtime.
Suggested action: Add targeted tests: (a) verify retry delay progression matches 6s/12s/24s/48s (GPT) and 9s/18s/36s/72s (Nemotron); (b) simulate two concurrent turns both receiving auto_retry events; (c) inject 503/ECONNRESET errors and verify retry contract; (d) integration-style test that workflow uploads artifacts/posts comment then fails on analysis outcome.
Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
Verification: Run `npx vitest run test/advisor-session-runner.test.ts test/pr-review-advisor-workflow-boundary.test.ts` and verify new test blocks pass.
Missing regression test: Add the four behavior-specific tests described above.
Done when: The local improvement is applied, or the PR notes why it should be deferred.
Evidence: testDepth.verdict: runtime_validation_recommended New tests cover only base delay and success/exhaustion branches No tests for concurrent turn retry isolation or non-429 errors Workflow boundary validator checks ordering but no behavioral publish-then-fail test

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 or identify targeted runtime/integration validation for the changed behavior; do not report external E2E job pass/fail here.. Runtime/sandbox/infrastructure paths need behavioral runtime validation: .github/workflows/pr-review-advisor.yaml, tools/advisors/session.mts, tools/pr-review-advisor/README.md, tools/pr-review-advisor/workflow-boundary.mts.
PRA-T2 Runtime validation — Verify exponential backoff delay progression for both models (6s/12s/24s/48s and 9s/18s/36s/72s).. Runtime/sandbox/infrastructure paths need behavioral runtime validation: .github/workflows/pr-review-advisor.yaml, tools/advisors/session.mts, tools/pr-review-advisor/README.md, tools/pr-review-advisor/workflow-boundary.mts.
PRA-T3 Runtime validation — Simulate concurrent turn retry isolation (multiple turns hitting retries simultaneously).. Runtime/sandbox/infrastructure paths need behavioral runtime validation: .github/workflows/pr-review-advisor.yaml, tools/advisors/session.mts, tools/pr-review-advisor/README.md, tools/pr-review-advisor/workflow-boundary.mts.
PRA-T4 Runtime validation — Inject non-429 transient errors (503, ECONNRESET) and verify retry contract.. Runtime/sandbox/infrastructure paths need behavioral runtime validation: .github/workflows/pr-review-advisor.yaml, tools/advisors/session.mts, tools/pr-review-advisor/README.md, tools/pr-review-advisor/workflow-boundary.mts.
PRA-T5 Runtime validation — Integration-style test that workflow publishes artifacts/comments before outcome gate fails.. Runtime/sandbox/infrastructure paths need behavioral runtime validation: .github/workflows/pr-review-advisor.yaml, tools/advisors/session.mts, tools/pr-review-advisor/README.md, tools/pr-review-advisor/workflow-boundary.mts.
PRA-T6 Risk plan missing regression invariants for new retry/outcome logic — Add risk plan entries for: (a) retry contract invariant — maxRetries=4, provider.maxRetries=0, baseDelayMs model-specific; (b) workflow outcome step invariant — must run after artifact upload, if: always(), continue-on-error=false, reads steps.analysis.outcome.
PRA-T7 Retry and workflow outcome gate test coverage gaps — Add targeted tests: (a) verify retry delay progression matches 6s/12s/24s/48s (GPT) and 9s/18s/36s/72s (Nemotron); (b) simulate two concurrent turns both receiving auto_retry events; (c) inject 503/ECONNRESET errors and verify retry contract; (d) integration-style test that workflow uploads artifacts/posts comment then fails on analysis outcome.

Since last review details

Current findings, using the urgency labels above:

`PRA-1` Resolve/justify — Merge conflict risk with open PR #5755 on same workflow file

Location: .github/workflows/pr-review-advisor.yaml:1
Category: workflow
Problem: PR chore(deps): bump actions/checkout from 6.0.3 to 7.0.0 #5755 (chore(deps): bump actions/checkout from 6.0.3 to 7.0.0) modifies the same workflow file. If both PRs merge without coordination, the actions/checkout version pin could conflict with the new 'Verify advisor analysis outcome' step and retry logic added in this PR.
Impact: Merge conflict could silently drop the new workflow verification step or revert the checkout version, weakening the trusted-code boundary or breaking the pinned action requirement.
Recommended action: Coordinate with PR chore(deps): bump actions/checkout from 6.0.3 to 7.0.0 #5755 author to rebase or resolve conflict before either merges. Verify the final workflow retains both the updated checkout action pin and the new outcome verification step.
Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
Verification: Check PR chore(deps): bump actions/checkout from 6.0.3 to 7.0.0 #5755 diff against this PR's workflow changes; attempt a test merge to confirm no silent drops.
Missing regression test: Add a workflow boundary test that validates the actions/checkout version pin remains at a full commit SHA after merge with dependency updates.
Done when: The risk is fixed or explicitly justified in the PR. Verification: Check PR chore(deps): bump actions/checkout from 6.0.3 to 7.0.0 #5755 diff against this PR's workflow changes; attempt a test merge to confirm no silent drops.
Evidence: Open PR chore(deps): bump actions/checkout from 6.0.3 to 7.0.0 #5755 listed in openPrOverlaps with sameFiles: ['.github/workflows/pr-review-advisor.yaml'] This PR adds 'Verify advisor analysis outcome' step and retry logic to the same workflow

`PRA-2` Resolve/justify — Risk plan missing regression invariants for new retry/outcome logic

Location: tools/pr-review-advisor/workflow-boundary.mts:1
Category: tests
Problem: The deterministic risk plan (planHash e02bcef1) shows families: [] and requiredJobs: [] for this 6-file change. The PR adds: (1) provider-aware retry layer with 4-attempt exponential backoff that can mask transient failures, and (2) workflow outcome verification step that must run after artifact upload with continue-on-error=false. Neither has declared invariants or required jobs in the risk plan.
Impact: Without declared invariants, future changes could weaken the retry contract (e.g., increase maxRetries, change baseDelayMs) or reorder the outcome verification step before artifact upload without triggering deterministic regression checks.
Recommended action: Add risk plan entries for: (a) retry contract invariant — maxRetries=4, provider.maxRetries=0, baseDelayMs model-specific; (b) workflow outcome step invariant — must run after artifact upload, if: always(), continue-on-error=false, reads steps.analysis.outcome.
Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
Verification: Inspect the checked-in risk plan at the PR head commit; verify families/requiredJobs arrays are empty for this change.
Missing regression test: Add a test that asserts the risk plan for this commit includes invariants covering advisorRetrySettings() contract and workflow outcome step ordering.
Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the checked-in risk plan at the PR head commit; verify families/requiredJobs arrays are empty for this change.
Evidence: riskPlan.families: [] and riskPlan.requiredJobs: [] in scope/risk context New retry logic in session.mts: advisorRetrySettings() with maxRetries=4, provider.maxRetries=0 New workflow step 'Verify advisor analysis outcome' with boundary validation in workflow-boundary.mts

`PRA-3` Improvement — Retry and workflow outcome gate test coverage gaps

Location: test/advisor-session-runner.test.ts:1
Category: tests
Problem: Unit tests cover retry success/exhaustion and workflow boundary validation, but miss: (1) exponential backoff delay progression (2nd/3rd/4th retry delays), (2) concurrent turn retry isolation (multiple turns hitting retries), (3) non-429 transient provider errors (5xx, network timeout), (4) behavioral test that workflow publishes artifacts/comments before outcome gate fails.
Impact: Without these tests, regressions in retry timing, turn isolation, error classification, or workflow publish-before-fail behavior could go undetected until runtime.
Suggested action: Add targeted tests: (a) verify retry delay progression matches 6s/12s/24s/48s (GPT) and 9s/18s/36s/72s (Nemotron); (b) simulate two concurrent turns both receiving auto_retry events; (c) inject 503/ECONNRESET errors and verify retry contract; (d) integration-style test that workflow uploads artifacts/posts comment then fails on analysis outcome.
Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
Verification: Run `npx vitest run test/advisor-session-runner.test.ts test/pr-review-advisor-workflow-boundary.test.ts` and verify new test blocks pass.
Missing regression test: Add the four behavior-specific tests described above.
Done when: The local improvement is applied, or the PR notes why it should be deferred.
Evidence: testDepth.verdict: runtime_validation_recommended New tests cover only base delay and success/exhaustion branches No tests for concurrent turn retry isolation or non-429 errors Workflow boundary validator checks ordering but no behavioral publish-then-fail test

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.

github-actions · 2026-07-09T18:36:26Z

PR Review Advisor — Changes requested

Merge posture: Do not merge yet
Primary next action: Resolve or justify PRA-1: Reset per-attempt advisor state when provider retry starts.
Open items: 0 required · 1 warning · 0 suggestions · 4 test follow-ups
Since last review: 0 prior items resolved · 1 still applies · 0 new items found

Action checklist

PRA-1 Resolve or justify: Reset per-attempt advisor state when provider retry starts in tools/advisors/session.mts:451
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: tools/advisors/session.mts provider retry recovery

Findings index

ID	Severity	Category	Location	Required action
`PRA-1`	Resolve/justify	correctness	`tools/advisors/session.mts:451`	Make retry attempts attempt-scoped: on auto_retry_start discard or snapshot failed-attempt text/flow/tool-success state and promote only the successful retry attempt, or otherwise explicitly reset all per-attempt mutable state that participates in final text and validation.

Review findings by urgency: 0 required fixes, 1 item 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 — Reset per-attempt advisor state when provider retry starts

Location: tools/advisors/session.mts:451
Category: correctness
Problem: Provider retry handling clears only the current turn error, while assistant text, turn-flow events, and successful tool names accumulated before auto_retry_start remain in the active turn. If a transient provider failure occurs after partial text or a successful required tool, the successful retry can return mixed failed-attempt and retry-attempt output, and required-tool validation can be satisfied by an abandoned attempt.
Impact: Transient provider recovery can weaken the advisor runner's state-machine guarantees by preserving stale abandoned-attempt output and tool success, potentially allowing a turn to pass atomic/required-tool validation based on events not produced by the final retry attempt.
Recommended action: Make retry attempts attempt-scoped: on auto_retry_start discard or snapshot failed-attempt text/flow/tool-success state and promote only the successful retry attempt, or otherwise explicitly reset all per-attempt mutable state that participates in final text and validation.
Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
Verification: Read tools/advisors/session.mts around the text/tool event handlers and auto_retry_start/auto_retry_end; confirm text/flow/tool-success state is retained across retry boundaries while only currentTurnError is reset.
Missing regression test: Add advisor-session-runner tests where the mock emits partial assistant text and where it emits a successful required tool before auto_retry_start, then succeeds on retry; assert the final turn text/flow and required-tool validation use only the successful retry attempt.
Done when: The risk is fixed or explicitly justified in the PR. Verification: Read tools/advisors/session.mts around the text/tool event handlers and auto_retry_start/auto_retry_end; confirm text/flow/tool-success state is retained across retry boundaries while only currentTurnError is reset.
Evidence: tools/advisors/session.mts appends text into currentTurnFlow/currentTurnText on text_delta events and records successful tools in successfulToolNames before retry handling. The auto_retry_start/auto_retry_end handlers clear currentTurnError but do not reset currentTurnText, currentTurnFlow, or successfulToolNames. New retry tests emit only stream/message error events before auto_retry_start and assert error clearing/exhaustion, not partial text or successful tool state before retry.

💡 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 — Add or identify targeted runtime/integration validation for the changed behavior; do not report external E2E job pass/fail here.. Runtime/sandbox/infrastructure paths need behavioral runtime validation: .github/workflows/pr-review-advisor.yaml, tools/advisors/session.mts, tools/pr-review-advisor/README.md, tools/pr-review-advisor/workflow-boundary.mts. Changed runtime/workflow advisor paths have focused unit and static boundary coverage, including retry success/exhaustion and workflow outcome gate regressions. The remaining confidence gap is behavior-specific retry-state coverage for partial failed-attempt text and successful tools before retry success.
PRA-T2 Runtime validation — Add an advisor-session-runner test where the mocked SDK emits partial assistant text before auto_retry_start, then succeeds on retry with different text; assert the final turn text and flow exclude the abandoned attempt.. Runtime/sandbox/infrastructure paths need behavioral runtime validation: .github/workflows/pr-review-advisor.yaml, tools/advisors/session.mts, tools/pr-review-advisor/README.md, tools/pr-review-advisor/workflow-boundary.mts. Changed runtime/workflow advisor paths have focused unit and static boundary coverage, including retry success/exhaustion and workflow outcome gate regressions. The remaining confidence gap is behavior-specific retry-state coverage for partial failed-attempt text and successful tools before retry success.
PRA-T3 Runtime validation — Add an advisor-session-runner test where the mocked SDK emits a successful required tool before auto_retry_start, then retry succeeds; assert required-tool validation is based on the successful retry attempt rather than stale successfulToolNames.. Runtime/sandbox/infrastructure paths need behavioral runtime validation: .github/workflows/pr-review-advisor.yaml, tools/advisors/session.mts, tools/pr-review-advisor/README.md, tools/pr-review-advisor/workflow-boundary.mts. Changed runtime/workflow advisor paths have focused unit and static boundary coverage, including retry success/exhaustion and workflow outcome gate regressions. The remaining confidence gap is behavior-specific retry-state coverage for partial failed-attempt text and successful tools before retry success.
PRA-T4 tools/advisors/session.mts provider retry recovery — Add mocked advisor-session-runner cases where partial assistant text and a successful required tool occur before auto_retry_start, then retry succeeds, and assert final output/validation use only the retry attempt.. auto_retry_start/auto_retry_end clear currentTurnError, while text/flow/tool-success buffers are populated outside an attempt scope and retained across retry events.

Since last review details

Current findings, using the urgency labels above:

`PRA-1` Resolve/justify — Reset per-attempt advisor state when provider retry starts

Location: tools/advisors/session.mts:451
Category: correctness
Problem: Provider retry handling clears only the current turn error, while assistant text, turn-flow events, and successful tool names accumulated before auto_retry_start remain in the active turn. If a transient provider failure occurs after partial text or a successful required tool, the successful retry can return mixed failed-attempt and retry-attempt output, and required-tool validation can be satisfied by an abandoned attempt.
Impact: Transient provider recovery can weaken the advisor runner's state-machine guarantees by preserving stale abandoned-attempt output and tool success, potentially allowing a turn to pass atomic/required-tool validation based on events not produced by the final retry attempt.
Recommended action: Make retry attempts attempt-scoped: on auto_retry_start discard or snapshot failed-attempt text/flow/tool-success state and promote only the successful retry attempt, or otherwise explicitly reset all per-attempt mutable state that participates in final text and validation.
Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
Verification: Read tools/advisors/session.mts around the text/tool event handlers and auto_retry_start/auto_retry_end; confirm text/flow/tool-success state is retained across retry boundaries while only currentTurnError is reset.
Missing regression test: Add advisor-session-runner tests where the mock emits partial assistant text and where it emits a successful required tool before auto_retry_start, then succeeds on retry; assert the final turn text/flow and required-tool validation use only the successful retry attempt.
Done when: The risk is fixed or explicitly justified in the PR. Verification: Read tools/advisors/session.mts around the text/tool event handlers and auto_retry_start/auto_retry_end; confirm text/flow/tool-success state is retained across retry boundaries while only currentTurnError is reset.
Evidence: tools/advisors/session.mts appends text into currentTurnFlow/currentTurnText on text_delta events and records successful tools in successfulToolNames before retry handling. The auto_retry_start/auto_retry_end handlers clear currentTurnError but do not reset currentTurnText, currentTurnFlow, or successfulToolNames. New retry tests emit only stream/message error events before auto_retry_start and assert error clearing/exhaustion, not partial text or successful tool state before retry.

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>

## Summary Adds model-aware same-session retries for transient PR Review Advisor provider failures, using one retry layer and staggered GPT/Nemotron backoff. The workflow still publishes summaries, comments, and artifacts after an incomplete analysis, then fails an explicit final outcome gate so the missing review cannot appear green. ## Changes - Configure four bounded agent-level retries while disabling the provider SDK's overlapping retry layer. - Clear transient attempt errors after a successful retry and preserve the final provider error when retries are exhausted. - Add a final workflow outcome check after summary, comment, and artifact publication, with boundary validation that prevents the failure from being masked. - Add retry and workflow regression tests and document the maintainer-facing behavior. ## 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  - [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: this changes internal PR Review Advisor behavior; `tools/pr-review-advisor/README.md` documents the maintainer-facing contract and no public NemoClaw behavior 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: - [ ] 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] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run check:diff` passed when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — command/result or justification: `npx vitest run test/advisor-session-runner.test.ts test/pr-review-advisor-workflow-boundary.test.ts test/pr-review-advisor.test.ts` (3 files, 74 tests passed); `npm run typecheck:cli`, `npm run source-shape:check`, and `npm run test-size:check` passed. - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: not applicable; the change is isolated to advisor retry and workflow-boundary behavior with focused coverage. - [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) ---  Signed-off-by: Carlos Villela <cvillela@nvidia.com>  ## Summary by CodeRabbit * **New Features** * Added bounded retry handling for transient advisor provider failures within the same session, improving reliability. * Added an explicit post-analysis verification step to ensure advisor results complete successfully. * **Bug Fixes** * Prevents the workflow from continuing when the analysis outcome is missing, not successful, or when verification is misconfigured. * Improves reporting when transient retries are exhausted. * **Documentation** * Updated the PR Review Advisor workflow documentation to reflect the new transient retry behavior and revised step ordering.  --------- Signed-off-by: Carlos Villela <cvillela@nvidia.com>

fix(advisor): retry transient provider failures

105391b

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

cv self-assigned this Jul 9, 2026

test(advisor): keep retry setup branch-free

fd977d7

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

cv merged commit c1bf898 into main Jul 9, 2026
47 checks passed

cv deleted the codex/advisor-retry-outcome branch July 9, 2026 19:05

github-actions Bot added the v0.0.79 Release target label Jul 9, 2026

Uh oh!

Conversation

cv commented Jul 9, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Type of Change

Quality Gates

Verification

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Jul 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

❌ Failed checks (1 warning)

Uh oh!

github-code-quality Bot commented Jul 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage Overview

TypeScript / code-coverage/plugin

TypeScript / code-coverage/cli

Uh oh!

github-actions Bot commented Jul 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

E2E Advisor Recommendation

E2E Recommendation Advisor

Required E2E

Optional E2E

New E2E recommendations

Uh oh!

github-actions Bot commented Jul 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

E2E Target Recommendation

E2E Target Advisor

Required E2E targets

Optional E2E targets

Relevant changed files

Uh oh!

github-actions Bot commented Jul 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Review Advisor (Nemotron Ultra) — Changes requested

Action checklist

Findings index

⚠️ Resolve or justify before merge

PRA-1 Resolve/justify — Merge conflict risk with open PR #5755 on same workflow file

PRA-2 Resolve/justify — Risk plan missing regression invariants for new retry/outcome logic

💡 In-scope improvements

PRA-3 Improvement — Retry and workflow outcome gate test coverage gaps

PRA-1 Resolve/justify — Merge conflict risk with open PR #5755 on same workflow file

PRA-2 Resolve/justify — Risk plan missing regression invariants for new retry/outcome logic

PRA-3 Improvement — Retry and workflow outcome gate test coverage gaps

Uh oh!

github-actions Bot commented Jul 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Review Advisor — Changes requested

Action checklist

Findings index

⚠️ Resolve or justify before merge

PRA-1 Resolve/justify — Reset per-attempt advisor state when provider retry starts

💡 In-scope improvements

PRA-1 Resolve/justify — Reset per-attempt advisor state when provider retry starts

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

cv commented Jul 9, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Jul 9, 2026 •

edited

Loading

github-code-quality Bot commented Jul 9, 2026 •

edited

Loading

github-actions Bot commented Jul 9, 2026 •

edited

Loading

github-actions Bot commented Jul 9, 2026 •

edited

Loading

github-actions Bot commented Jul 9, 2026 •

edited

Loading

`PRA-1` Resolve/justify — Merge conflict risk with open PR #5755 on same workflow file

`PRA-2` Resolve/justify — Risk plan missing regression invariants for new retry/outcome logic

`PRA-3` Improvement — Retry and workflow outcome gate test coverage gaps

`PRA-1` Resolve/justify — Merge conflict risk with open PR #5755 on same workflow file

`PRA-2` Resolve/justify — Risk plan missing regression invariants for new retry/outcome logic

`PRA-3` Improvement — Retry and workflow outcome gate test coverage gaps

github-actions Bot commented Jul 9, 2026 •

edited

Loading

`PRA-1` Resolve/justify — Reset per-attempt advisor state when provider retry starts

`PRA-1` Resolve/justify — Reset per-attempt advisor state when provider retry starts