feat(ci): automate E2E advisor and mock parity - #6583
Merged
Conversation
prekshivyas
added
area: ci
11 tasks
Contributor
|
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:
📝 WalkthroughWalkthroughThe PR adds pull_request_target handling to the E2E advisor workflow and introduces a mock-parity validation flow for live E2E tests in the PR and main E2E support workflows, with matching tests, docs, and ownership updates. ChangesFork PR handling in E2E Advisor
Live E2E mock parity validation
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
test/e2e-advisor.test.ts (1)
74-78: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMock
ghuses"$*", which loses argument-boundary fidelity.
"$*"joins arguments with a single space, so if any real invocation argument later contains internal whitespace, the logged command and thetoContainassertion could pass/fail spuriously without reflecting actual arg boundaries."$@"piped through a delimiter-aware join (or an array dump) would be more robust, though not required for the current fixed argument set.🤖 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/e2e-advisor.test.ts` around lines 74 - 78, The fake gh wrapper in the test currently logs arguments with "$*", which collapses argument boundaries and can hide whitespace-related issues. Update the mock command in the test setup to record arguments using a delimiter-aware approach based on "$@" so each argument stays distinct when written to FAKE_GH_LOG, keeping the assertions in test/e2e-advisor.test.ts faithful to the real invocation..github/workflows/e2e-advisor.yaml (1)
112-132: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winResolve the PR from the commit directly.
GET /repos/{owner}/{repo}/commits/{sha}/pullsalready returns PRs associated with that SHA, including open fork PRs; keep thehead.repo.full_name/base.repo.full_namefilters and the exactly-one check.🤖 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 @.github/workflows/e2e-advisor.yaml around lines 112 - 132, The PR lookup in the workflow should be resolved directly from the commit SHA instead of listing all open pulls; update the `gh api` call in the fork-PR resolution step to use the commit-to-PR endpoint tied to `HEAD_SHA`, then keep the existing `jq` filters on `head.repo.full_name` and `base.repo.full_name` plus the exactly-one count check in the same block that sets `pr_number` and `base_ref`.
🤖 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/e2e-advisor.yaml:
- Around line 96-137: The fork PR lookup in the Resolve fork PR from completed
CI head step is matching github.event.workflow_run.head_sha against
pulls[].head.sha, but for pull_request workflow runs that SHA is the merge
commit, so the lookup can fail. Update the workflow_run handling to resolve the
PR by associated PR number or by a persisted true head SHA from the CI run, and
then use that resolved PR data in the jq filtering and validation logic.
---
Nitpick comments:
In @.github/workflows/e2e-advisor.yaml:
- Around line 112-132: The PR lookup in the workflow should be resolved directly
from the commit SHA instead of listing all open pulls; update the `gh api` call
in the fork-PR resolution step to use the commit-to-PR endpoint tied to
`HEAD_SHA`, then keep the existing `jq` filters on `head.repo.full_name` and
`base.repo.full_name` plus the exactly-one count check in the same block that
sets `pr_number` and `base_ref`.
In `@test/e2e-advisor.test.ts`:
- Around line 74-78: The fake gh wrapper in the test currently logs arguments
with "$*", which collapses argument boundaries and can hide whitespace-related
issues. Update the mock command in the test setup to record arguments using a
delimiter-aware approach based on "$@" so each argument stays distinct when
written to FAKE_GH_LOG, keeping the assertions in test/e2e-advisor.test.ts
faithful to the real invocation.
🪄 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: 66a01c9d-c46e-41f8-bab8-515bea2d79cc
📒 Files selected for processing (2)
.github/workflows/e2e-advisor.yamltest/e2e-advisor.test.ts
prekshivyas
force-pushed
the
ci/fork-e2e-advisor
branch
from
July 9, 2026 16:55
884ccf9 to
5115c9c
Compare
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)
.github/workflows/e2e-advisor.yaml (1)
1-13: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winRemove the unnecessary
issues: writescope
pull-requests: writeis enough for the PR comment posting steps here;issues: writeis broader than needed for thispull_request_targetjob.🤖 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 @.github/workflows/e2e-advisor.yaml around lines 1 - 13, The E2E Advisor workflow is requesting a broader token permission than needed. Update the permissions block in the E2E / Advisor workflow so the job keeps the PR comment capability via pull-requests: write but removes the unnecessary issues: write scope, using the workflow’s permissions configuration near the pull_request_target setup.Source: Path instructions
🤖 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 @.github/workflows/e2e-advisor.yaml:
- Around line 1-13: The E2E Advisor workflow is requesting a broader token
permission than needed. Update the permissions block in the E2E / Advisor
workflow so the job keeps the PR comment capability via pull-requests: write but
removes the unnecessary issues: write scope, using the workflow’s permissions
configuration near the pull_request_target setup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: aedb8218-7881-40d3-9572-75f369c342dd
📒 Files selected for processing (2)
.github/workflows/e2e-advisor.yamltest/e2e-advisor.test.ts
✅ Files skipped from review due to trivial changes (1)
- test/e2e-advisor.test.ts
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
prekshivyas
force-pushed
the
ci/fork-e2e-advisor
branch
from
July 9, 2026 17:06
5115c9c to
8b0d49c
Compare
prekshivyas
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
scripts/checks/e2e-mock-parity.ts (1)
44-108: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting per-entry validation into a helper.
validateMockParitymixes manifest-shape validation, per-entry validation, and changed-file cross-checking in one function with several nested branches. Splitting entry validation into a dedicated helper (e.g.validateEntry(entry, fileExists): string[]) would reduce cyclomatic complexity and improve testability of each rule in isolation.As per coding guidelines, "Keep function complexity low in JavaScript and TypeScript code."
🤖 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 `@scripts/checks/e2e-mock-parity.ts` around lines 44 - 108, `validateMockParity` is doing manifest validation, per-entry rule checks, and changed-file cross-checks all in one place, making the function too complex. Extract the per-entry logic from `validateMockParity` into a dedicated helper such as `validateEntry(entry, fileExists): string[]`, and have `validateMockParity` call it for each `MockParityEntry` while keeping the manifest-shape and changed-file checks separate. Preserve the existing behavior and error messages, and use the `validateMockParity` and new helper symbols to keep the rules easy to locate and test.Source: Coding guidelines
🤖 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/main.yaml:
- Around line 173-177: The parity check step in the workflow needs to handle the
zero-SHA base case because github.event.before can be all zeros on the first
push and there may be no valid HEAD~1 base. Update the Validate changed live E2E
mock parity step to detect this case before calling
scripts/checks/e2e-mock-parity.ts, and either skip the check or substitute a
safe existing base ref when BASE_SHA is zero. Use the BASE_SHA env and the
workflow step name to locate the logic.
In `@scripts/checks/e2e-mock-parity.ts`:
- Around line 84-89: The live-only validation in e2e-mock-parity.ts is calling
trim() on entry.liveOnlyReason without first verifying it is a string. Update
the validation around the entry/liveOnlyReason checks to type-check
liveOnlyReason the same way fast is validated, and only call trim() after
confirming it is a string; otherwise push a validation error instead of letting
a TypeError escape.
---
Nitpick comments:
In `@scripts/checks/e2e-mock-parity.ts`:
- Around line 44-108: `validateMockParity` is doing manifest validation,
per-entry rule checks, and changed-file cross-checks all in one place, making
the function too complex. Extract the per-entry logic from `validateMockParity`
into a dedicated helper such as `validateEntry(entry, fileExists): string[]`,
and have `validateMockParity` call it for each `MockParityEntry` while keeping
the manifest-shape and changed-file checks separate. Preserve the existing
behavior and error messages, and use the `validateMockParity` and new helper
symbols to keep the rules easy to locate and test.
🪄 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: cf3f56f0-f1ae-467e-a0e1-6e45dc46a636
📒 Files selected for processing (10)
.github/CODEOWNERS.github/workflows/e2e-advisor.yaml.github/workflows/main.yaml.github/workflows/pr.yamlscripts/checks/e2e-mock-parity.tstest/e2e-advisor.test.tstest/e2e-mock-parity.test.tstest/e2e/docs/README.mdtest/e2e/mock-parity.jsontest/pr-workflow-contract.test.ts
✅ Files skipped from review due to trivial changes (2)
- test/e2e/docs/README.md
- test/pr-workflow-contract.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- test/e2e-advisor.test.ts
- .github/workflows/e2e-advisor.yaml
19 tasks
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/e2e-advisor.test.ts (1)
272-290: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider generalizing the secret-ordering check.
This test only validates ordering for the two hardcoded advisor step names. A future step that references
secrets.*before the symlink-removal step wouldn't be caught. Since this is testing a security-relevant workflow contract (secrets shouldn't be exposed to unstripped analysis content), consider scanning all steps forsecrets.references and asserting they occur afterremoveSymlinksIndex, rather than enumerating specific step names.🤖 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/e2e-advisor.test.ts` around lines 272 - 290, Generalize the secret-ordering assertion in readAdvisorWorkflow-based test so it checks every workflow step that references secrets.* instead of only the two hardcoded advisor steps. Update the logic in the e2e advisor test to scan the steps array, find any step with secret-bearing env or run content, and assert each such step appears after the "Remove symlinks from analysis workspace" step. Keep the existing removeSymlinksIndex lookup and use it as the security boundary for all secret-consuming steps.Source: Path instructions
🤖 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 `@test/e2e-advisor.test.ts`:
- Around line 272-290: Generalize the secret-ordering assertion in
readAdvisorWorkflow-based test so it checks every workflow step that references
secrets.* instead of only the two hardcoded advisor steps. Update the logic in
the e2e advisor test to scan the steps array, find any step with secret-bearing
env or run content, and assert each such step appears after the "Remove symlinks
from analysis workspace" step. Keep the existing removeSymlinksIndex lookup and
use it as the security boundary for all secret-consuming steps.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 68ecbf6c-f6ad-4b71-910a-990ebb06e9f4
📒 Files selected for processing (2)
.github/workflows/e2e-advisor.yamltest/e2e-advisor.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/e2e-advisor.yaml
Collaborator
|
Exact head |
Collaborator
|
Maintainer exact-head follow-up for
Nonblocking follow-ups: generalize the secret-ordering contract test to cover every future |
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Collaborator
Author
|
@cv Exact-head rereview requested for I addressed the actionable follow-up from your
Exact-head evidence:
Your other two follow-ups are correctly post-merge/operational: the real fork/symlink canary requires the merged |
cv
approved these changes
Jul 10, 2026
Hadar301
pushed a commit
to Hadar301/NemoClaw-OpenShift
that referenced
this pull request
Jul 12, 2026
<!-- markdownlint-disable MD041 --> ## Summary Runs the E2E Advisor for fork PRs in parallel with normal PR CI and adds a deterministic changed-live-test parity gate. Fork analysis executes trusted code from `main`; new or modified live E2Es must identify a fast PR-collected counterpart or an explicit live-only reason. ## Related Issue Part of NVIDIA#6145. ## Changes - Add a parallel `pull_request_target` path for fork PRs while preserving the existing `pull_request` path for first-party PRs. - Bind fork analysis to the triggering head SHA, execute advisor code exclusively from `NVIDIA/NemoClaw:main`, and remove worktree symlinks before secrets enter the analysis step. - Add `test/e2e/mock-parity.json` as the small live-to-fast mapping manifest. - Fail PR and `main` `e2e-support` lanes when a new or modified live test lacks a valid fast-test mapping or explicit `liveOnlyReason`. - Validate mapped file existence and ensure counterparts belong to a fast PR-collected Vitest project. - Document the changed-file rollout; untouched historical live tests do not require an immediate backfill. ## 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: The privileged event executes only the workflow and Node implementation from trusted `main`. PR content is fetched through the base repository pull ref, verified against the event head SHA, stripped of symlinks, and exposed only to repo-confined read-only tools. No PR script, install hook, action, or executable is invoked. - [ ] 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 — 15/15 Advisor boundary tests passed; 4/4 parity behaviors passed; focused PR/main workflow contract passed; the parity CLI passed against the PR diff. - [ ] Applicable broad gate passed — not applicable; focused workflow and validator tests cover the changed behavior, and normal CI runs the complete Linux `e2e-support` lane. - [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) - [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) --- Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * E2E Advisor now supports additional workflow triggers (including fork-safe pull request handling) and posts advisor comments for those runs too. * Added an E2E mock parity check to ensure changed live E2E tests match the expected “fast PR” mock contract. * **Bug Fixes** * Fixed base/head reference handling for advisor runs, and hardens the workflow by stripping untrusted symlinks before secret-bearing steps. * **Tests** * Expanded workflow and parity-check coverage, including updated assertions and new validation tests. * **Documentation** * Documented the “changed-file policy” and location of `mock-parity.json`. * **Chores** * Updated workflow permissions and ensured full git history where needed. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> Co-authored-by: Carlos Villela <cvillela@nvidia.com>
16 tasks
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
3 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
Runs the E2E Advisor for fork PRs in parallel with normal PR CI and adds a deterministic changed-live-test parity gate. Fork analysis executes trusted code from
main; new or modified live E2Es must identify a fast PR-collected counterpart or an explicit live-only reason.Related Issue
Part of #6145.
Changes
pull_request_targetpath for fork PRs while preserving the existingpull_requestpath for first-party PRs.NVIDIA/NemoClaw:main, and remove worktree symlinks before secrets enter the analysis step.test/e2e/mock-parity.jsonas the small live-to-fast mapping manifest.maine2e-supportlanes when a new or modified live test lacks a valid fast-test mapping or explicitliveOnlyReason.Type of Change
Quality Gates
main. PR content is fetched through the base repository pull ref, verified against the event head SHA, stripped of symlinks, and exposed only to repo-confined read-only tools. No PR script, install hook, action, or executable is invoked.Verification
Verifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailablee2e-supportlane.npm run docsbuilds without warnings (doc changes only)Signed-off-by: Prekshi Vyas prekshiv@nvidia.com
Summary by CodeRabbit
mock-parity.json.