fix(ci): route advisors to inference API - #5384
Conversation
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
📝 WalkthroughWalkthroughThis PR consolidates advisor credential handling by migrating from optional credential fallbacks to explicit environment variable sources, implementing per-turn error tracking in the advisor session, and enforcing credential boundaries in CI workflows. The session module now captures assistant errors, workflow steps use dedicated API key secrets, and analyzers validate error state before proceeding. ChangesAdvisor credential consolidation and error tracking
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
E2E Advisor RecommendationRequired E2E: None Full advisor summaryE2E Recommendation AdvisorFailed: Could not parse JSON from advisor output; see /home/runner/work/NemoClaw/NemoClaw/artifacts/e2e-advisor/e2e-advisor-raw-output.txt |
Vitest E2E Scenario RecommendationRequired Vitest E2E scenarios: None Full Vitest E2E advisor summaryVitest E2E Scenario AdvisorFailed: Could not parse JSON from advisor output; see /home/runner/work/NemoClaw/NemoClaw/artifacts/e2e-advisor/e2e-scenario-advisor-raw-output.txt |
PR Review AdvisorFindings: 0 needs attention, 1 worth checking, 0 nice ideas Review findings🛠️ Needs attention
🔎 Worth checking
🌱 Nice ideas
Consider writing more tests for
This is an automated advisory review. A human maintainer must make the final merge decision. |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in the Show a code coverage summary of the most covered files.
TypeScript / code-coverage/cliThe overall coverage in the Show a code coverage summary of the most covered files.
Updated |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tools/advisors/session.mts (1)
143-153: 💤 Low valueConsider guarding against duplicate error capture within the same turn.
The
currentTurnError ||= normalizedpattern captures only the first error per turn, which is reasonable. However, subsequent errors for the same turn are still logged torawbut silently discarded fromturnErrors. If multiple distinct errors occur in a turn, only the first is surfaced to callers.This is likely intentional (first error is usually the root cause), but worth confirming the design intent.
🤖 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 `@tools/advisors/session.mts` around lines 143 - 153, The current captureTurnError function sets currentTurnError only once but continues to append every subsequent error to raw and never records them in turnErrors; to guard against duplicate capture, change captureTurnError (the function using currentTurnError, currentTurnText, turnErrors, raw and options.logPrefix) so that if currentTurnError is already set it returns immediately (or at least skips raw.append and any push into turnErrors), and only when currentTurnError is undefined should it set currentTurnError ||= normalized, append the formatted message to raw and add the normalized error to turnErrors; this ensures only the first distinct error per turn is logged/recorded while preventing duplicate captures for the same turn.
🤖 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.
Nitpick comments:
In `@tools/advisors/session.mts`:
- Around line 143-153: The current captureTurnError function sets
currentTurnError only once but continues to append every subsequent error to raw
and never records them in turnErrors; to guard against duplicate capture, change
captureTurnError (the function using currentTurnError, currentTurnText,
turnErrors, raw and options.logPrefix) so that if currentTurnError is already
set it returns immediately (or at least skips raw.append and any push into
turnErrors), and only when currentTurnError is undefined should it set
currentTurnError ||= normalized, append the formatted message to raw and add the
normalized error to turnErrors; this ensures only the first distinct error per
turn is logged/recorded while preventing duplicate captures for the same turn.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 387c9d76-c34c-41b1-9624-50be428d0385
📒 Files selected for processing (10)
.github/workflows/e2e-advisor.yaml.github/workflows/pr-review-advisor.yamltest/pr-review-advisor.test.tstools/advisors/session.mtstools/e2e-advisor/README.mdtools/e2e-advisor/analyze.mtstools/e2e-advisor/scenarios.mtstools/pr-review-advisor/README.mdtools/pr-review-advisor/analyze.mtstools/pr-review-advisor/workflow-boundary.mts
💤 Files with no reviewable changes (1)
- .github/workflows/e2e-advisor.yaml
wscurran
added
area: ci
2 participants
Summary
Route the PR and E2E advisor SDK configuration to the hosted OpenAI-compatible
inference-api.nvidia.comservice so the injected advisor secrets are used against the intended endpoint. The advisor wrappers now surface provider errors in artifacts/comments instead of collapsing empty SDK output into a JSON parse failure.Changes
https://inference-api.nvidia.com/v1with conservative OpenAI-compatible request flags.PI_PR_REVIEW_ADVISOR_API_KEYandOPENAI_API_KEYadvisor fallbacks from workflows and docs.Type of Change
Verification
npx prek run --all-filespassesnpm testpassesnpm run docsbuilds without warnings (doc changes only)Signed-off-by: Carlos Villela cvillela@nvidia.com
Summary by CodeRabbit
Bug Fixes
Documentation