fix(advisor): preserve complete PR diffs - #8149
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesAdvisor reliability updates
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant getDiff
participant Git
participant Advisor
getDiff->>Git: Request three-dot diff
Git-->>getDiff: Return diff or command failure
getDiff->>Git: Request two-dot diff when needed
Git-->>getDiff: Return complete diff or failure
getDiff-->>Advisor: Return complete diff or throw
sequenceDiagram
participant runPrReviewAdvisorAnalysis
participant runAnalysis
participant ResultFiles
runPrReviewAdvisorAnalysis->>runAnalysis: Start analysis
runAnalysis-->>runAnalysis: Detect analysis failure
runAnalysis->>ResultFiles: Write failure artifacts
runAnalysis-->>runPrReviewAdvisorAnalysis: Throw analysis error
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit 72b61de in the TypeScript / code-coverage/cliThe overall coverage in commit 72b61de in the Show a code coverage summary of the most impacted files.
Updated |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tools/pr-review-advisor/analyze.mts`:
- Line 363: Update tools/pr-review-advisor/analyze.mts around getDiff and
writeFailure so errors during pre-session Git setup still produce the required
failure artifacts; ensure writeFailure is available before getDiff can throw and
route both-command failures through that path. In
test/pr-review-advisor-diff.test.ts, add coverage for failure of both diff
commands and for a successful three-dot-to-two-dot fallback.
🪄 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: 18a67c50-ee33-48a8-8bdc-903e77ac7d15
📒 Files selected for processing (4)
test/pr-review-advisor-diff.test.tstools/advisors/git.mtstools/pr-review-advisor/README.mdtools/pr-review-advisor/analyze.mts
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
Second-opinion terminology and E2E selections are advisory. They do not change the primary assessment or E2E / PR Gate. 1 semantic terminology decisionTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: None 1 warning · 0 suggestionsWarningsWarnings do not block.
|
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tools/pr-review-advisor/run-analysis.mts`:
- Around line 280-284: Update the nonzero-exit fallback in
tools/pr-review-advisor/run-analysis.mts:280-284 around writeBootstrapResult so
it preserves existing partial or summary artifacts, writes only missing fallback
artifacts, and still throws the original RunAnalysisError. In
test/pr-review-advisor-workflow-boundary.test.ts:896-925, pre-create a partial
artifact and verify fallback completion preserves it while producing the
remaining artifacts.
🪄 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: 625a403b-f46c-45de-922f-3b4d68f59d07
📒 Files selected for processing (3)
test/pr-review-advisor-diff.test.tstest/pr-review-advisor-workflow-boundary.test.tstools/pr-review-advisor/run-analysis.mts
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tools/pr-review-advisor/analyze.mts (1)
399-438: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPre-session failure path now writes artifacts; add coverage for the double-resolution-failure fallback.
This correctly resolves the earlier critical issue:
getChangedFiles,getHeadSha,getDiff, andcollectDeterministicContextfailures now write pre-session failure artifacts before the original error propagates.One branch still lacks direct test coverage. At lines 418-424, when
getHeadSha(headRef)also fails during recovery,headShafalls back to"unavailable". No test exercises this branch (the current test intest/pr-review-advisor-diff.test.tsonly makesgetChangedFilesfail whileHEADstill resolves). Add a case with both--baseand--headpointing at unresolved refs to confirm the failure artifacts still get written withheadSha: "unavailable".🤖 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/pr-review-advisor/analyze.mts` around lines 399 - 438, Add a test in the PR review advisor failure-path coverage that invokes the CLI with both base and head refs unresolved, causing the initial context collection and the recovery getHeadSha call to fail. Assert that failure artifacts are still written and their metadata records headSha as "unavailable", while preserving propagation of the original failure.
🤖 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/pr-review-advisor/analyze.mts`:
- Around line 399-438: Add a test in the PR review advisor failure-path coverage
that invokes the CLI with both base and head refs unresolved, causing the
initial context collection and the recovery getHeadSha call to fail. Assert that
failure artifacts are still written and their metadata records headSha as
"unavailable", while preserving propagation of the original failure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d8b445aa-3d13-4f9d-b53a-3e0da0d961d1
📒 Files selected for processing (2)
test/pr-review-advisor-diff.test.tstools/pr-review-advisor/analyze.mts
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
apurvvkumaria
left a comment
apurvvkumaria
left a comment
There was a problem hiding this comment.
Reviewed exact head 72b61de. The change preserves complete diffs and failure artifacts and I found no blocking defect. Non-blocking fast follow: bound or stream PR-controlled diff capture so an unusually large diff cannot exhaust the trusted advisor process before it writes its failure artifact. Functional CI passes; aggregate and E2E gates remain unresolved.
4 participants
Summary
The PR Review Advisor previously shortened diffs at 160,000 characters before the first review turn, so later turns could reuse incomplete evidence. It now captures the complete Git diff and fails if Git cannot provide it. Existing tests checked context transport only with small diffs; the regression test now exceeds the former boundary.
Changes
Type of Change
Quality Gates
Documentation Writer Review
docs-updatedtools/pr-review-advisor/README.mddescribes complete diff delivery and pre-session failure artifacts; no Fern page covers this internal maintainer workflow.DGX Station Hardware Evidence
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailablenpm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result:npm run docsbuilds without warnings (doc changes only)Additional validation:
npm run typecheck:cli,npm run test-size:check, andnpm run test:changedpassed.npm run docspassed with zero errors and two Fern warnings; no Fern source files changed.Signed-off-by: Julie Yaunches jyaunches@nvidia.com
Summary by CodeRabbit
Bug Fixes
Documentation
Tests