Skip to content

chore(skills): batch review findings before push - #8533

Merged
cv merged 1 commit into
mainfrom
chore/batch-review-findings
Aug 7, 2026
Merged

chore(skills): batch review findings before push#8533
cv merged 1 commit into
mainfrom
chore/batch-review-findings

Conversation

@cv

@cv cv commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator

Summary

Changes PR repair guidance from a per-finding edit loop to one complete review cycle. Agents now collect and classify review evidence, group findings by root cause, model sensitive failure states, review every corrected commit, and push once after the final review and recollection pass.

Changes

  • Require repository-bound, complete review evidence with PR and local revision identity, pagination status, required-check results, and retained-evidence disposition.
  • Group findings by root cause and inspect unchanged sibling paths that implement the same operation or failure class.
  • Require a state matrix for credential, remote-execution, cost, cleanup, policy, and external-write workflows.
  • Require documentation reviewers to complete every applicable category after finding a blocker.
  • Require every corrected HEAD to pass independent review before one final push.
  • Add phrase-based policy tests for the contributor workflow contracts.

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 contributor-agent workflows and does not change a supported user-facing product surface.
  • 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: not applicable; no product sensitive path changed.
  • Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: no failed local check was accepted.

Documentation Writer Review

  • Documentation writer subagent reviewed the completed changes
  • Result: docs-updated
  • Evidence:
    • .agents/skills/_shared/documentation-writing-review.md
    • .agents/skills/_shared/pr-follow-up.md
    • .agents/skills/nemoclaw-contributor-create-pr/SKILL.md
    • Review result: PASS with no findings on 2babbe47e.
  • Agent: Pi CLI

DGX Station Hardware Evidence

  • Tested on DGX Station
  • Tested commit:
  • Station profile/scenario:
  • Result:
  • Supporting evidence:

Verification

  • PR description includes a Signed-off-by: line and every commit appears as Verified in GitHub
  • 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
  • Targeted behavior tests pass for the current change set, or tests are marked not applicable above — npx vitest run --project integration test/maintainer-skills-policy.test.ts test/skills-frontmatter.test.ts (2 files, 57 tests passed)
  • Applicable broad gate passed — not run; npm run checks:repository passed.
  • 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)

Additional focused checks:

  • npm run checks:repository
  • npx --yes markdownlint-cli2@0.22.0 '.agents/skills/_shared/*.md' '.agents/skills/nemoclaw-contributor-create-pr/**/*.md'
  • git diff --check
  • Independent documentation writer review: PASS with no findings on 2babbe47e

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

Summary by CodeRabbit

  • Documentation

    • Strengthened pull request review guidance with complete evidence collection, stable commit tracking, comprehensive check coverage, and clearer handling of review findings.
    • Added structured procedures for grouping related fixes, validating changes, reviewing documentation, and safely completing sensitive workflows.
    • Improved contributor guidance to require final review confirmation before changes are pushed.
  • Tests

    • Added coverage for review completeness, failure handling, evidence cleanup, grouped repairs, and push restrictions.

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

coderabbitai Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

The PR strengthens PR review closure procedures. It adds complete, head-stable evidence collection, grouped finding resolution, sensitive-workflow checks, final-commit documentation review, push gating, and policy tests.

Changes

Review closure workflow

Layer / File(s) Summary
Review coverage and PR binding
.agents/skills/_shared/documentation-writing-review.md, .agents/skills/_shared/pr-follow-up.md
Review guidance now requires full-diff coverage, adjacent-path inspection, repository-bound PR state, and complete review status checks.
Evidence collection and grouped repair
.agents/skills/_shared/pr-follow-up.md
The workflow now collects paginated, head-stable evidence, classifies and groups findings, models sensitive workflow states, and validates the final reviewed head before pushing.
Contributor workflow and policy validation
.agents/skills/nemoclaw-contributor-create-pr/SKILL.md, test/maintainer-skills-policy.test.ts
Contributor instructions require final review evidence before push. Tests cover evidence completeness, grouped fixes, documentation review, failure handling, and push restrictions.

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

Sequence Diagram(s)

sequenceDiagram
  participant Contributor
  participant PRFollowUpWorkflow
  participant GitHubReviewEvidence
  participant Repository
  Contributor->>PRFollowUpWorkflow: Start review collection
  PRFollowUpWorkflow->>GitHubReviewEvidence: Collect paginated PR evidence
  GitHubReviewEvidence-->>PRFollowUpWorkflow: Return head-stable findings and checks
  PRFollowUpWorkflow->>Repository: Apply grouped fixes and validation
  PRFollowUpWorkflow->>GitHubReviewEvidence: Recollect evidence for final head
  GitHubReviewEvidence-->>PRFollowUpWorkflow: Return final review receipt
  PRFollowUpWorkflow->>Repository: Push when no actionable findings remain
Loading

Possibly related PRs

Suggested labels: area: docs

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: batching review findings before pushing contributor workflow updates.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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 chore/batch-review-findings

Warning

Tools execution failed with the following error:

Failed to run tools: 14 UNAVAILABLE: Connection dropped


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

@github-code-quality

github-code-quality Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Code Coverage Overview

Languages: TypeScript

TypeScript / code-coverage/plugin

The overall coverage in commit 2babbe4 in the chore/batch-review-f... branch remains at 96%, unchanged from commit df56c02 in the main branch.

TypeScript / code-coverage/cli

The overall coverage in commit 2babbe4 in the chore/batch-review-f... branch remains at 81%, unchanged from commit df56c02 in the main branch.

Show a code coverage summary of the most impacted files.
File main df56c02 chore/batch-review-f... 2babbe4 +/-
src/lib/onboard...flight-ports.ts 67% 33% -34%
src/lib/messagi...ink-base-url.ts 100% 90% -10%
src/lib/messagi...onfig-parser.ts 100% 93% -7%
src/lib/trace.ts 94% 90% -4%
src/lib/messagi...onfig-parser.ts 97% 93% -4%
src/lib/credentials/store.ts 56% 55% -1%
src/lib/messagi...flow-planner.ts 91% 91% 0%
src/lib/onboard...-transaction.ts 93% 93% 0%
src/lib/agent/d...e-base-image.ts 86% 100% +14%
src/lib/core/pr...mpt-activity.ts 67% 92% +25%

Updated August 07, 2026 06:32 UTC

@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 @.agents/skills/_shared/pr-follow-up.md:
- Around line 88-90: Update Step 1 of the latest completed head-stable
collection to gather the complete result set for every required check, including
passing, pending, cancelled, skipped, and failed states, rather than only
failures. Preserve recording each check’s source commit when available, or the
collected head SHA otherwise, and ensure the complete required-check set is
available for the final collection and push decision.
🪄 Autofix

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: a0004a1c-5f80-4c6f-a9b6-68a072ae755b

📥 Commits

Reviewing files that changed from the base of the PR and between df56c02 and 2babbe4.

📒 Files selected for processing (4)
  • .agents/skills/_shared/documentation-writing-review.md
  • .agents/skills/_shared/pr-follow-up.md
  • .agents/skills/nemoclaw-contributor-create-pr/SKILL.md
  • test/maintainer-skills-policy.test.ts

Comment on lines +88 to +90
Before editing, collect and classify all review signals in the latest completed head-stable collection:

1. Re-read `headRefOid`. Collect current required-check failures, issue comments, submitted reviews, inline threads with resolution state, advisor findings, and required independent-review findings. Record each source commit when GitHub provides it; otherwise record the collected head SHA.

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.

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Collect every required-check result in the complete review cycle.

Line 70 requires the collection to record every required check, including pending, cancelled, and skipped results. Step 1 narrows this to required-check failures. A pending, cancelled, or skipped required check can therefore be absent from the final collection and push decision. Use the complete result set in Step 1.

Proposed wording
-1. Re-read `headRefOid`. Collect current required-check failures, issue comments, submitted reviews, inline threads with resolution state, advisor findings, and required independent-review findings.
+1. Re-read `headRefOid`. Collect the status, conclusion, and evaluated commit of every required check, including pending, cancelled, and skipped results; issue comments, submitted reviews, inline threads with resolution state, advisor findings, and required independent-review findings.
📝 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
Before editing, collect and classify all review signals in the latest completed head-stable collection:
1. Re-read `headRefOid`. Collect current required-check failures, issue comments, submitted reviews, inline threads with resolution state, advisor findings, and required independent-review findings. Record each source commit when GitHub provides it; otherwise record the collected head SHA.
Before editing, collect and classify all review signals in the latest completed head-stable collection:
1. Re-read `headRefOid`. Collect the status, conclusion, and evaluated commit of every required check, including pending, cancelled, and skipped results; issue comments, submitted reviews, inline threads with resolution state, advisor findings, and required independent-review findings. Record each source commit when GitHub provides it; otherwise record the collected head SHA.
🤖 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 @.agents/skills/_shared/pr-follow-up.md around lines 88 - 90, Update Step 1
of the latest completed head-stable collection to gather the complete result set
for every required check, including passing, pending, cancelled, skipped, and
failed states, rather than only failures. Preserve recording each check’s source
commit when available, or the collected head SHA otherwise, and ensure the
complete required-check set is available for the final collection and push
decision.

@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

PR Review Advisor — No blocking findings reported

Advisor assessment: No blocking advisor findings reported
Next action: No advisor follow-up needed.
Findings: 0 blockers · 0 warnings · 0 suggestions

Model lanes

  • GPT-5.6 Terra (primary): Completed · high confidence · 0 blockers · 0 warnings · 0 suggestions
  • Nemotron 3 Ultra (second opinion): Completed · high confidence · 0 blockers · 0 warnings · 0 suggestions
  • Model comparison: normalized findings match; normalized terminology decisions differ; normalized E2E selections match; severity counts match.
8 terminology differences from the second opinion

Advisory only. These are normalized differences from the primary terminology receipt.

  • blocked at .agents/skills/_shared/pr-follow-up.md:80: selected only by the second-opinion lane as established.
  • independent documentation writer review at .agents/skills/_shared/pr-follow-up.md:134: selected only by the second-opinion lane as established.
  • root-cause at .agents/skills/_shared/documentation-writing-review.md:28: selected only by the second-opinion lane as established.
  • retained evidence at .agents/skills/_shared/pr-follow-up.md:84: selected only by the second-opinion lane as established.
  • headRefOid at .agents/skills/_shared/pr-follow-up.md:63: selected only by the second-opinion lane as established.
  • Complete Review Cycle at .agents/skills/_shared/pr-follow-up.md:86: selected only by the second-opinion lane as define.
  • head-stable at .agents/skills/_shared/pr-follow-up.md:88: selected only by the second-opinion lane as define.
  • candidate change set at .agents/skills/_shared/pr-follow-up.md:133: selected only by the second-opinion lane as define.

Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests.

4 semantic terminology decisions

Terminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.

  • define — head-stable collection at .agents/skills/_shared/pr-follow-up.md:88: Define the term at first use or retain the adjacent headRefOid stability requirement as its operational definition.
  • define — sensitive-workflow state matrix at .agents/skills/_shared/pr-follow-up.md:98: Retain the adjacent scope and outcome table as the term definition.
  • define — retained collection evidence at .agents/skills/_shared/pr-follow-up.md:96: Retain the adjacent artifact removal and absence-verification requirements as the term definition.
  • established — actionable finding at .agents/skills/_shared/pr-follow-up.md:139: Use the established term consistently for findings that require action or disposition.

E2E guidance

Advisory only. A maintainer can dispatch the default E2E suite against this exact revision.

Recommended E2E: None

Workflow run details

This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge.

@cv
cv merged commit 16a302b into main Aug 7, 2026
67 of 68 checks passed
@cv
cv deleted the chore/batch-review-findings branch August 7, 2026 07:12
@github-actions github-actions Bot added the v0.0.105 Release target label Aug 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

v0.0.105 Release target

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant