feat(advisor): add PR review advisor workflow#3834
Merged
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
Contributor
|
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 (5)
📝 WalkthroughWalkthroughAdds shared advisor utilities (git, GitHub, IO, JSON, session), refactors e2e advisor to use them, implements a PR Review Advisor CLI that normalizes AI output against a JSON schema, an accompanying comment poster, a GitHub Actions workflow to run the advisor, and comprehensive tests. ChangesPR Review Advisor & Shared Infrastructure
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Comment |
Contributor
E2E Advisor RecommendationRequired E2E: None Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
|
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
test/pr-review-advisor.test.ts (1)
184-189: ⚡ Quick winAdd existence assertions before accessing
step.run.If a step name changes, these dereferences can fail with a
TypeErrorbefore the intended assertion runs. Add explicit presence checks to keep failures clear and actionable.Proposed patch
+ expect(installStep).toBeDefined(); + expect(analyzeStep).toBeDefined(); + expect(commentStep).toBeDefined(); + if (!installStep || !analyzeStep || !commentStep) return; + expect(installStep.run.includes("--ignore-scripts")).toBe(true); expect(analyzeStep.run.includes("$ADVISOR_DIR/tools/pr-review-advisor/analyze.mts")).toBe(true); expect(analyzeStep.run).toContain("trusted main checkout does not yet contain analyze.mts"); expect(analyzeStep.run).toContain("pr-review-advisor-final-result.json"); expect(commentStep.run).toContain("trusted main checkout does not yet contain comment.mts");🤖 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 `@test/pr-review-advisor.test.ts` around lines 184 - 189, Add explicit presence assertions for each step object before accessing their .run property to avoid TypeError when a step name changes: for installStep, analyzeStep, and commentStep assert they are defined/truthy (e.g., expect(installStep).toBeDefined()/toBeTruthy(), same for analyzeStep and commentStep) immediately prior to the existing expect(... .run ...) checks so failures clearly indicate a missing step rather than throwing while dereferencing .run.
🤖 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 @.github/workflows/pr-review-advisor.yaml:
- Line 94: Replace the loose tags for the GitHub Actions with full commit SHAs:
change uses: actions/setup-node@v6 and uses: actions/upload-artifact@v4 to pin
to the corresponding full commit SHA for each repo (as was done for checkout).
Locate the official action repositories (actions/setup-node and
actions/upload-artifact), copy the desired release/commit hash, and update the
workflow lines so they read uses: actions/setup-node@<full-sha> and uses:
actions/upload-artifact@<full-sha>.
In `@tools/advisors/github.mts`:
- Around line 47-49: The githubGraphql function should parse the JSON body even
when response.ok is true, then check if the parsed payload has a non-empty
payload.errors array; if so, throw an Error (or a custom Error) that includes
the GraphQL errors and attach the full payload to the error (e.g., error.payload
= payload) so callers can still inspect ["data","..."] if needed; otherwise
return the full payload object (not payload.data). Update githubGraphql to await
response.json() into a variable (e.g., payload), inspect payload.errors, and
either throw with the payload attached or return payload.
In `@tools/pr-review-advisor/analyze.mts`:
- Around line 431-433: The current ternary treats "unstable" and "HAS_HOOKS" as
pass; change the logic so only clean states (e.g., /CLEAN|clean/i) produce
status "pass" and move /UNSTABLE|unstable|HAS_HOOKS/i into the warning branch.
Update the condition around mergeState (the regex tests that produce { status:
"pass", ... } and the subsequent /DIRTY|CONFLICT|BLOCKED|behind/i test) so that
UNSTABLE and HAS_HOOKS no longer match the pass regex and instead produce {
status: "warning", evidence: `mergeStateStatus=${mergeState}` }.
In `@tools/pr-review-advisor/schema.json`:
- Around line 1-4: The JSON file is missing the required SPDX header; add SPDX
metadata as a top-of-file comment string(s) before the JSON content including
SPDX-FileCopyrightText with the copyright owner and year and
SPDX-License-Identifier: Apache-2.0 so the file begins with the SPDX header
followed by the existing JSON object (which contains keys like "$schema", "$id",
and "title") to satisfy the repo policy for all .json source files.
---
Nitpick comments:
In `@test/pr-review-advisor.test.ts`:
- Around line 184-189: Add explicit presence assertions for each step object
before accessing their .run property to avoid TypeError when a step name
changes: for installStep, analyzeStep, and commentStep assert they are
defined/truthy (e.g., expect(installStep).toBeDefined()/toBeTruthy(), same for
analyzeStep and commentStep) immediately prior to the existing expect(... .run
...) checks so failures clearly indicate a missing step rather than throwing
while dereferencing .run.
🪄 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: 59b45ac7-e5f7-468a-9f20-4faf5575b6e0
📒 Files selected for processing (15)
.github/workflows/pr-review-advisor.yamltest/pr-review-advisor.test.tstools/advisors/README.mdtools/advisors/git.mtstools/advisors/github.mtstools/advisors/io.mtstools/advisors/json.mtstools/advisors/session.mtstools/e2e-advisor/analyze.mtstools/e2e-advisor/comment.mtstools/e2e-advisor/dispatch.mtstools/pr-review-advisor/README.mdtools/pr-review-advisor/analyze.mtstools/pr-review-advisor/comment.mtstools/pr-review-advisor/schema.json
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
jyaunches
approved these changes
May 20, 2026
12 tasks
12 tasks
miyoungc
added a commit
that referenced
this pull request
May 21, 2026
## Summary Refreshes NemoClaw release notes for v0.0.47 and v0.0.48, then regenerates the corresponding user-skill references so agent-facing docs match the source pages. Preview: https://nvidia-preview-docs-release-notes-47-48.docs.buildwithfern.com/nemoclaw/about/release-notes ## Changes - Adds explicit v0.0.47 and v0.0.48 sections to `docs/about/release-notes.mdx`. - Documents follow-up WSL Ollama, sandbox image, share mount, and troubleshooting updates from recent release changes. - Regenerates `nemoclaw-user-*` skill references from the Fern MDX source docs. ## Source Summary - #4003 -> `docs/about/release-notes.mdx`: Notes the messaging manifest registry work as part of v0.0.48 release coverage. - #3984 -> `docs/about/release-notes.mdx`: Captures Hermes messaging policy scoping in the v0.0.48 release notes. - #3963 -> `docs/about/release-notes.mdx`: Captures DGX Spark Hermes GPU recreation startup recovery in the v0.0.48 release notes. - #3961 -> `docs/about/release-notes.mdx`: Captures Discord loopback proxy routing in the v0.0.48 release notes. - #3940 -> `docs/about/release-notes.mdx`: Captures installer prompt clarification and express-install behavior in the v0.0.48 release notes. - #3946 -> `docs/about/release-notes.mdx`: Carries forward the Homebrew preinstall clarification in release coverage. - #3937 -> `docs/about/release-notes.mdx`: Carries forward the dashboard URL command and post-install next steps coverage. - #3921 -> `docs/about/release-notes.mdx`: Carries forward managed vLLM default behavior for DGX Spark and DGX Station. - #3931 -> `docs/about/release-notes.mdx`, `docs/reference/architecture.mdx`: Documents the sandbox `python` to `python3` compatibility symlink. - #1485 -> `docs/about/release-notes.mdx`, `docs/reference/architecture.mdx`: Documents the sandbox image Docker health check. - #3784 -> `docs/about/release-notes.mdx`: Captures VM-driver snapshot health-check reliability in release notes. - #3917 -> `docs/about/release-notes.mdx`: Captures package-based workspace template resolution in release notes. - #3170 -> `docs/about/release-notes.mdx`: Captures installer checksum compatibility from preferring `sha256sum`. - #3898 -> `docs/about/release-notes.mdx`: Adds v0.0.47 release coverage for messaging provider scenario validation. - #3897 -> `docs/about/release-notes.mdx`: Adds v0.0.47 release coverage for baseline onboarding scenario validation. - #3834 -> `docs/about/release-notes.mdx`: Adds v0.0.47 release coverage for PR review advisor automation. - #3838 -> `docs/about/release-notes.mdx`: Adds v0.0.47 release coverage for CLI display registry refactoring. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [x] Doc only (includes code sample changes) ## Verification - [x] `npx prek run --all-files` passes - [ ] `npm test` passes - [ ] Tests added or updated for new or changed behavior - [x] No secrets, API keys, or credentials committed - [x] Docs updated for user-facing behavior changes - [ ] `make docs` builds without warnings (doc changes only) - [x] 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) `make docs` was attempted but could not complete because `npx fern-api` failed with `403 Forbidden` from `https://registry.npmjs.org/fern-api` in this environment. Pre-commit and pre-push hooks passed after refreshing the local CLI build output with `npm run build:cli`; no build artifacts were committed. --- Signed-off-by: Miyoung Choi <miyoungc@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Added WSL onboarding notes for Windows-host Ollama detection, restart guidance, and PowerShell checks. * Clarified express-install behavior (non-interactive, sudo prompts) and default sandbox policy selection. * Added Windows preparation guidance when installer tooling is missing (winget/App Installer or Docker Desktop). * Expanded sandbox docs with Docker health checks, Homebrew/python compatibility helpers, share-mount path validation, Discord troubleshooting, and new v0.0.48/v0.0.47 release notes. * **Chores** * Improved docs preview workflow error handling. <!-- review_stack_entry_start --> [](https://app.coderabbit.ai/change-stack/NVIDIA/NemoClaw/pull/4007?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack) <!-- review_stack_entry_end --> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
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
4 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
Adds a trusted, read-only PR Review Advisor workflow modeled on the existing E2E Advisor. The new advisor analyzes PR diffs and GitHub metadata for NemoClaw-specific review signals, then posts an advisory sticky comment without approving, merging, dispatching workflows, or executing PR-provided code.
Changes
tools/pr-review-advisor/with the analyzer, sticky-comment updater, JSON schema, and README..github/workflows/pr-review-advisor.yamlwith the same trusted-code boundary pattern ase2e-advisor.yaml.test/pr-review-advisor.test.tscoverage for normalization, schema validation, comment rendering, test-depth classification, and workflow safety structure.Type of Change
Verification
npx prek run --all-filespassesnpm testpassesmake docsbuilds without warnings (doc changes only)Additional verification run locally:
npm run build:clinpm run typecheck:clinpm test -- --run test/pr-review-advisor.test.tsnpm run source-shape:checkPR_REVIEW_ADVISOR_RUN_ANALYSIS=0 node --experimental-strip-types tools/pr-review-advisor/analyze.mts --base main --head HEAD --schema tools/pr-review-advisor/schema.json --out-dir /tmp/pr-review-advisor-smokeKnown local pre-push/full-suite status: the full CLI test hook failed on existing environment/timeout-sensitive tests unrelated to this change, including
test/cli.test.ts,test/sandbox-connect-inference.test.ts,test/dns-proxy.test.ts,test/generate-openclaw-config.test.ts, andtest/secret-redaction.test.ts. Targeted advisor tests and type-checking passed.Signed-off-by: Carlos Villela cvillela@nvidia.com
Summary by CodeRabbit
New Features
Documentation
Tests