fix(advisor): keep review findings actionable - #6682
Merged
Conversation
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Contributor
📝 WalkthroughWalkthroughThe PR Review Advisor now designates one publishing lane, keeps other advisor lanes artifact-only, loads prior reviews only for the publishing lane, narrows deterministic signals, updates prompt context, and renders findings-focused summaries and comments. ChangesPR Review Advisor workflow and publishing contract
Deterministic context and review protocol
Findings synthesis and comment rendering
Ledger and depth test fixtures
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested labels: Sequence Diagram(s)sequenceDiagram
participant AdvisorMatrix
participant ReviewJob
participant ReviewAdvisor
participant CommentStep
AdvisorMatrix->>ReviewJob: select publishing or artifact-only lane
ReviewJob->>ReviewAdvisor: run advisor analysis
ReviewAdvisor->>ReviewJob: return findings and summary
ReviewJob->>CommentStep: publish comment only for publishing lane
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage remains at 96%, unchanged from the TypeScript / code-coverage/cliThe overall coverage in the Show a code coverage summary of the most impacted files.
Updated |
Contributor
E2E Advisor RecommendationRequired E2E: None Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
|
Contributor
PR Review Advisor — Changes requestedMerge posture: Do not merge yet Action checklist
Findings index
Review findings by urgency: 0 required fixes, 2 items to resolve/justify, 0 in-scope improvements
|
Collaborator
Author
|
PRA-1 is intentionally not adopted. It is based on a file-size threshold, not an observed defect. Removing that heuristic is one of this PR’s intended outcomes: file size may prompt inspection, but it should not create a finding by itself. This run used the trusted advisor from current |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tools/pr-review-advisor/workflow-boundary.mts (1)
188-201: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMissing validation for the new
PR_REVIEW_ADVISOR_WORKFLOW_NAMEfixed value.
COMMENT_MARKER,COMMENT_TITLE, andCOMMENT_LABELare all asserted against fixed constants here, but the newPR_REVIEW_ADVISOR_WORKFLOW_NAMEenv var (set to"PR Review / Advisor"in the workflow) has no matchingrequireJobEnvValuecheck. This value is used to match workflow runs for previous-review loading, so an accidental drift between the workflow's actual name and this literal would go undetected by the boundary validator.🛡️ Proposed fix
requireJobEnvValue(errors, reviewJob, "PR_REVIEW_ADVISOR_COMMENT_TITLE", "PR Review Advisor"); requireJobEnvValue(errors, reviewJob, "PR_REVIEW_ADVISOR_COMMENT_LABEL", "PR review advisor"); + requireJobEnvValue( + errors, + reviewJob, + "PR_REVIEW_ADVISOR_WORKFLOW_NAME", + "PR Review / Advisor", + ); requireJobEnvValue( errors, reviewJob, "PR_REVIEW_ADVISOR_LOAD_PREVIOUS_REVIEW", "${{ matrix.advisor.publish_comment }}", );🤖 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/workflow-boundary.mts` around lines 188 - 201, Add a requireJobEnvValue validation in the workflow-boundary checks for PR_REVIEW_ADVISOR_WORKFLOW_NAME, asserting the fixed value "PR Review / Advisor" alongside the existing COMMENT_MARKER, COMMENT_TITLE, and COMMENT_LABEL validations. Keep the existing publish-comment validation unchanged.
🤖 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.
Outside diff comments:
In `@tools/pr-review-advisor/workflow-boundary.mts`:
- Around line 188-201: Add a requireJobEnvValue validation in the
workflow-boundary checks for PR_REVIEW_ADVISOR_WORKFLOW_NAME, asserting the
fixed value "PR Review / Advisor" alongside the existing COMMENT_MARKER,
COMMENT_TITLE, and COMMENT_LABEL validations. Keep the existing publish-comment
validation unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 93449138-5aca-4bcb-b74b-674be724db24
📒 Files selected for processing (10)
.github/workflows/pr-review-advisor.yamltest/pr-review-advisor-ledger-tools.test.tstest/pr-review-advisor-test-depth.test.tstest/pr-review-advisor-turns.test.tstest/pr-review-advisor-workflow-boundary.test.tstest/pr-review-advisor.test.tstools/pr-review-advisor/README.mdtools/pr-review-advisor/analyze.mtstools/pr-review-advisor/comment.mtstools/pr-review-advisor/workflow-boundary.mts
💤 Files with no reviewable changes (1)
- test/pr-review-advisor-turns.test.ts
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Collaborator
Author
|
PRA-2 is also intentional. A warning identifies a concrete concern worth maintainer attention, but only blockers determine merge posture. Requiring every warning to be resolved or justified recreates the coercive behavior this PR removes. Any finding severe enough to require author action should be classified as a blocker. |
Hadar301
pushed a commit
to Hadar301/NemoClaw-OpenShift
that referenced
this pull request
Jul 12, 2026
<!-- markdownlint-disable MD041 --> ## Summary The advisor currently turns discussion, heuristics, and internal test ideas into repeated author work. This change limits findings to concrete defects, makes only blockers affect merge posture, and publishes one concise review while keeping the second model as an independent artifact-only evaluation. ## Changes - Ground acceptance findings in observable outcomes, current constraints, supported contracts, and explicit maintainer decisions; treat proposals and ordinary discussion as context. - Require one concrete present defect per root cause, keep test-depth ideas internal, and remove keyword and file-size heuristics that created speculative findings. - Render each finding once, with warnings non-blocking and suggestions optional; keep concrete test coverage in the finding it belongs to. - Publish one primary sticky comment. Keep the comparison lane non-blocking, artifact-only, and independent of the primary lane's previous review. - Scope process-boundary test-depth signals to added runtime source lines and retain workflow boundary checks for publishing, failure handling, and prior-review context. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [x] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [x] Docs updated for user-facing behavior changes - [ ] Docs not applicable — justification: - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: Independent review covered prompt eligibility, workflow trust boundaries, model-lane isolation, escaping, and failure behavior; no concrete defects remained. - [ ] 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: advisor suite (6 files, 112 tests passed); after the automated review fix, `npx vitest run --project integration test/pr-review-advisor-workflow-boundary.test.ts` (1 file, 14 tests passed) - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: - [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> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - PR reviews now clearly separate required blockers (merge posture), non-blocking warnings (maintainer attention), and optional suggestions. - PR review comments use a simplified “findings” format with streamlined verification guidance. - Only the configured publishing lane posts PR comments; other lanes remain artifact-only. - **Bug Fixes** - Improved heuristics for runtime-change detection, dependency/simplification signals, and issue-reference extraction. - Unavailable/warning-only outcomes no longer block merges. - **Documentation** - Updated the PR Review Advisor README to reflect the refined scope, workflow behavior, artifacts, and output semantics. - **Tests** - Updated review-advisor fixtures and boundary/gating expectations to match the new behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Carlos Villela <cvillela@nvidia.com>
wscurran
added
area: ci
20 tasks
jyaunches
added a commit
that referenced
this pull request
Aug 19, 2026
<!-- markdownlint-disable MD041 --> ## Summary The PR Review Advisor should block material codebase-design defects without sending authors through review-driven growth loops. This change keeps the evidence and anti-noise safeguards from #6682 and #6832, makes maintainability defects first-class blockers, and requires simplicity feedback to point toward a smaller complete source-and-test design. ## Changes - Define duplicated authority, unnecessary machinery, repeated setup, widened dependencies, unrelated churn, and unsupported product scope as present design defects when checked-in evidence supports a behavior-preserving reduction. - Require blocker severity when the advisor expects author action before merge. Keep warnings optional only when maintainers can reasonably accept the current design without author action. - Require an unnecessary-complexity finding to name its reduction case across source and tests. Prefer a negative total line delta; permit a line-neutral result only for a material reduction in owners, concepts, invalid combinations, or dependency width. - Reject net-new helpers, registries, configuration surfaces, compatibility layers, fallbacks, migrations, test frameworks, and parallel fixture owners as simplification remedies unless current consumers adopt them and the complete change removes more structure than it adds. - Preserve semantic regression coverage while allowing duplicated fixtures, matrices, assertions, and test owners to be consolidated. - Keep follow-on findings grounded in current code, scope ownership findings to checked-in duplication, and classify merge-order-only redundancy as superseded or a limitation. - Keep a prior design root cause stable when a review response adds machinery instead of creating a serial chain of architecture findings. - Apply the same deletion and consolidation questions in the shared code-change considerations and the issue implementation skill so agents optimize the design before publication. - Add prompt, shared-guidance, and skill contract coverage for the new direction and retained anti-noise rules. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [x] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests 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: ## DGX Station Hardware Evidence - [ ] Tested on DGX Station - Tested commit: - Station profile/scenario: - Result: - Supporting evidence: ## Verification - [x] PR description includes a `Signed-off-by:` line and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run validate:pr` passed after refreshing `origin/main` when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — `npm exec -- vitest run test/code-change-considerations.test.ts test/pr-review-advisor-writing-guides.test.ts test/pr-review-advisor-context.test.ts test/skills-frontmatter.test.ts` (82 passed); `npm run test:changed` (growth guardrails: 32 passed; no changed CLI, plugin, or E2E-support tests) - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: - [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 doc pages only) --- Signed-off-by: Julie Yaunches <jyaunches@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Improvements** * Review guidance now more consistently identifies material design, security, authorization, scope, duplication, and complexity issues as blockers when supported by evidence. * Simplification recommendations favor reducing source and test complexity, avoiding added mechanisms and unsupported future-use justifications. * Checks better distinguish current defects from hypothetical risks, resolved behavior, and non-actionable concerns. * Terminology reviews and retry handling now provide clearer, more reliable outcomes. * **Documentation** * Clarified expectations for documenting reductions, line changes, preserved regression coverage, ownership evidence, and independent follow-on risks. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The advisor currently turns discussion, heuristics, and internal test ideas into repeated author work. This change limits findings to concrete defects, makes only blockers affect merge posture, and publishes one concise review while keeping the second model as an independent artifact-only evaluation.
Changes
Type of Change
Quality Gates
Verification
Verifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailablenpx vitest run --project integration test/pr-review-advisor-workflow-boundary.test.ts(1 file, 14 tests passed)npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result:npm run docsbuilds without warnings (doc changes only)Signed-off-by: Carlos Villela cvillela@nvidia.com
Summary by CodeRabbit