ci(advisors): consolidate PR and E2E guidance#6745
Conversation
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
cv
added
security
area: ci
|
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 consolidates E2E guidance into the PR Review Advisor, moves analysis to a target-safe workflow with separate publication, adds deterministic E2E normalization and provenance validation, retires standalone E2E advisor surfaces, and removes advisor output from deterministic maintainer and comparator gates. ChangesUnified PR Review Advisor
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage remains at 96%, unchanged from the branch. TypeScript / code-coverage/cliThe overall coverage in the branch remains at 79%, unchanged from the branch. Show a code coverage summary of the most impacted files.
Updated |
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
tools/pr-review-advisor/comment.mts (1)
291-291: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated
20-item render cap into a named constant.The truncation limit is duplicated 5 times; a future change to one occurrence but not the others silently produces inconsistent truncation behavior across coverage/target/recommendation lists.
♻️ Proposed fix
+const MAX_RENDERED_E2E_ITEMS = 20; + function renderE2eDetails(result?: ReviewAdvisorResult): string { ... - for (const item of requiredTargets.slice(0, 20)) { + for (const item of requiredTargets.slice(0, MAX_RENDERED_E2E_ITEMS)) { ... - for (const item of optionalCoverage.slice(0, 20)) { + for (const item of optionalCoverage.slice(0, MAX_RENDERED_E2E_ITEMS)) { ... - for (const item of optionalTargets.slice(0, 20)) { + for (const item of optionalTargets.slice(0, MAX_RENDERED_E2E_ITEMS)) { ... - for (const item of newRecommendations.slice(0, 20)) { + for (const item of newRecommendations.slice(0, MAX_RENDERED_E2E_ITEMS)) { } function renderE2eIds(items: Array<{ id?: string }>): string { return items - .slice(0, 20) + .slice(0, MAX_RENDERED_E2E_ITEMS) .map((item) => `<code>${escapeLocationHtml(item.id || "unnamed")}</code>`) .join(", "); }As per path instructions for
tools/{advisors,pr-review-advisor}/**: "Derive inventories and limits from a canonical source where possible; flag duplicated lists that can silently drift."Also applies to: 308-308, 313-313, 318-318, 342-342
🤖 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/comment.mts` at line 291, Extract the shared 20-item truncation limit into a named constant near the relevant logic, then replace the repeated literal in the loops at lines 291, 308, 313, 318, and 342 with that constant. Keep all existing list-rendering behavior unchanged while ensuring future limit changes are centralized.Source: Path instructions
.agents/skills/nemoclaw-maintainer-day/scripts/pra-gate.ts (1)
208-262: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the
pull_request_targetvalidation clauses into named predicates.The 10+-clause compound condition (lines 240-255) is correct but dense for a single boolean expression. Splitting into small named checks (e.g.
workflowIdentityMatches,associationMatches) would improve testability of each binding rule independently, per the guideline to keep function complexity low.As per coding guidelines: "Keep TypeScript function complexity low and use the repository Biome formatting and linting configuration."
🤖 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/nemoclaw-maintainer-day/scripts/pra-gate.ts around lines 208 - 262, The pull_request_target branch of validateAdvisorRun contains a dense compound validation condition that should be decomposed. Extract the workflow identity checks and pull-request association checks into small named predicates near validateAdvisorRun, then invoke them from validateAdvisorRun while preserving every existing validation rule and Biome-compatible formatting.Source: Coding guidelines
test/pr-review-advisor-workflow-boundary.test.ts (1)
169-177: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor: fixture uses
coverage.required/targets.requiredinstead ofrequiredTests.
validPrimaryResult()modelscoveragewith arequiredkey rather than the schema'srequiredTests. It still passes the publish validator (which only checks object-ness), so tests are correct, but the fixture no longer resembles a real result shape and could mask a future stricter validator. Consider mirroring the canonical field 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/pr-review-advisor-workflow-boundary.test.ts` around lines 169 - 177, Update validPrimaryResult() so the e2e coverage and targets fixtures use the schema’s canonical requiredTests field instead of required, while preserving the existing empty-array values and overall result structure.
🤖 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:
- Around line 260-263: Update the skip-path result generation around the e2e
result object to reuse the shared normalizeCombinedE2eResult/result-builder flow
instead of constructing pr-review-advisor-final-result.json manually. Pass the
existing skip reason and empty test/target data through that normalizer so the
output includes sourceOfTruthReview and all required coverage and target fields
defined by schema.json.
In `@test/pr-review-advisor-provenance.test.ts`:
- Around line 12-27: Update collectTrustedPreviousAdvisorReview and its callers
to accept and validate the live current PR base SHA in addition to the run
snapshot’s pull_requests[].base.sha. Ensure provenance is rejected when the live
base differs from the hidden comment base, and extend mockRun/tests with a
retargeted or rebased case proving stale advisor reviews are not accepted.
In `@tools/pr-review-advisor/comment.mts`:
- Around line 285-299: Update the required coverage rendering near
renderE2eIds(requiredCoverage) to preserve and display each required test’s
reason, mirroring the detail loop used for requiredTargets. Iterate over the
requiredCoverage items and render their IDs with escaped reasons while retaining
the existing _None_ fallback and required-target output.
In `@tools/pr-review-advisor/README.md`:
- Line 59: Update the README wording to hyphenate “E2E-required status check” in
the workflow description, without changing the surrounding guidance.
---
Nitpick comments:
In @.agents/skills/nemoclaw-maintainer-day/scripts/pra-gate.ts:
- Around line 208-262: The pull_request_target branch of validateAdvisorRun
contains a dense compound validation condition that should be decomposed.
Extract the workflow identity checks and pull-request association checks into
small named predicates near validateAdvisorRun, then invoke them from
validateAdvisorRun while preserving every existing validation rule and
Biome-compatible formatting.
In `@test/pr-review-advisor-workflow-boundary.test.ts`:
- Around line 169-177: Update validPrimaryResult() so the e2e coverage and
targets fixtures use the schema’s canonical requiredTests field instead of
required, while preserving the existing empty-array values and overall result
structure.
In `@tools/pr-review-advisor/comment.mts`:
- Line 291: Extract the shared 20-item truncation limit into a named constant
near the relevant logic, then replace the repeated literal in the loops at lines
291, 308, 313, 318, and 342 with that constant. Keep all existing list-rendering
behavior unchanged while ensuring future limit changes are centralized.
🪄 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: 223d293a-132e-45bd-ae4d-e0ee82ce5a50
📒 Files selected for processing (37)
.agents/skills/nemoclaw-maintainer-day/MERGE-GATE.md.agents/skills/nemoclaw-maintainer-day/scripts/check-gates.ts.agents/skills/nemoclaw-maintainer-day/scripts/pra-gate.ts.coderabbit.yaml.github/workflows/e2e-advisor.yaml.github/workflows/pr-review-advisor.yamlci/source-shape-test-budget.jsonsrc/lib/onboard/gateway-tcp-readiness.tstest/e2e-advisor.test.tstest/e2e-recommendations.test.tstest/e2e/README.mdtest/e2e/docs/README.mdtest/e2e/support/e2e-operations-workflow-boundary.test.tstest/pr-review-advisor-provenance.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.tstest/skills/check-gates-pra.test.tstools/advisors/README.mdtools/advisors/e2e-recommendations.mtstools/e2e-advisor/README.mdtools/e2e-advisor/analyze.mtstools/e2e-advisor/comment.mtstools/e2e-advisor/schema.jsontools/e2e-advisor/target-comment.mtstools/e2e-advisor/targets-schema.jsontools/e2e-advisor/targets.mtstools/e2e-advisor/workflow-boundary.mtstools/e2e/credential-free-tests.mtstools/e2e/module-tags.mtstools/e2e/operations-workflow-boundary.mtstools/pr-review-advisor/README.mdtools/pr-review-advisor/analyze.mtstools/pr-review-advisor/comment.mtstools/pr-review-advisor/schema.jsontools/pr-review-advisor/workflow-boundary.mts
💤 Files with no reviewable changes (11)
- test/e2e-advisor.test.ts
- tools/e2e-advisor/comment.mts
- tools/e2e-advisor/schema.json
- tools/e2e-advisor/workflow-boundary.mts
- .github/workflows/e2e-advisor.yaml
- tools/e2e-advisor/targets-schema.json
- tools/e2e-advisor/analyze.mts
- tools/e2e-advisor/target-comment.mts
- tools/e2e-advisor/README.md
- ci/source-shape-test-budget.json
- tools/e2e-advisor/targets.mts
|
From an independent agent review: VerdictDo not merge #6745 as-is. The good news: prompt injection alone does not give the advisor shell access, arbitrary file access, or the publisher’s write token. The model is limited to repository-confined read tools, and publishing happens in a separate job. However, I found three merge-blocking problems and several availability/integrity risks.
Severity | Risk | Exploitation requirement
-- | -- | --
Blocker | Advisor suggests running E2E from the PR branch, where attacker-controlled code can receive secrets/write permissions | Maintainer follows command; same-repo malicious branch
Blocker | Advisor hard-gate provenance is incompatible with live GitHub metadata, while missing comments pass the gate | Normal operation or marker-squatting attacker
High, conditional | find can download and execute floating fd code while the model key and GitHub token are present | Prompt triggers find plus upstream/download compromise
Medium | Prompt injection can produce an authenticated-looking false merge_as_is result | Successful model manipulation
Medium | Fork PRs can consume two secret-backed model lanes repeatedly | Multiple PRs or synchronizations
Medium/Low | Malformed model output can crash or suppress the consolidated review | Prompt-controlled schema/output edge cases
What the PR gets rightThe core architecture is substantially safer than the deleted E2E advisor:
So the immediate operational rule is: do not run advisor-suggested E2E commands containing Verification notesReviewed exact head
All 37 PR files were inspected:
Supporting trust-boundary files and the exact Pi runtime implementation were also inspected, including Do not merge [#6745](#6745) as-is. The good news: prompt injection alone does not give the advisor shell access, arbitrary file access, or the publisher’s write token. The model is limited to repository-confined read tools, and publishing happens in a separate job. However, I found three merge-blocking problems and several availability/integrity risks.
1. E2E recommendations can become a human-assisted “pwn request”The advisor emits: from NemoClaw/.github/workflows/e2e.yaml Lines 82 to 112 in d9cb9f2 For a malicious same-repository branch, following that command can run PR-controlled workflow, test, package, or script code with repository secrets. A live repository-settings query also returned: {
"default_workflow_permissions": "write",
"can_approve_pull_request_reviews": true
}So a modified branch workflow could potentially request write permissions and alter repository state, subject to branch protection. This is the exact class of risk GitHub warns about when secrets and untrusted code meet. GitHub’s I also reproduced two allowlist/parser problems:
This unsafe command predates #6745, but the PR preserves it and promotes it into the consolidated review comment. Required fix: Never recommend PR-ref workflow dispatches. Route recommendations through the trusted 2. The advisor gate is broken in both directionsThe new validator requires NemoClaw/.agents/skills/nemoclaw-maintainer-day/scripts/pra-gate.ts Lines 208 to 262 in d9cb9f2 Live GitHub runs contradict both assumptions:
Therefore legitimate target-event comments will fail the documented hard gate in observed same-repository and fork shapes. At the same time, <!-- nemoclaw-pr-review-advisor -->The sticky-comment helper selects the first marker-bearing comment without checking its author, tries unsuccessfully to edit it, swallows the 403, and never creates a bot-owned comment. [Sticky-comment helper]( NemoClaw/tools/advisors/github.mts Lines 123 to 179 in d9cb9f2 The temporary legacy Required fix:
3. A “read-only” tool can execute floating native code with credentialsThe analysis step explicitly receives the model API key and a read-scoped GitHub token. It installs pinned Pi and ripgrep versions, but not NemoClaw/.github/workflows/pr-review-advisor.yaml Lines 212 to 233 in d9cb9f2 I inspected the exact
A PR author cannot choose that download URL, so prompt injection alone is insufficient. But an injection can trigger Required fix: Install and verify an exact 4. Prompt injection can corrupt the trusted review signalThe system prompt correctly says PR content is untrusted, but that is guidance, not a deterministic boundary. [Advisor system prompt]( NemoClaw/tools/pr-review-advisor/analyze.mts Lines 1723 to 1762 in d9cb9f2 The ledger proves that final output matches what the model previously entered into its own ledger; it does not prove factual completeness. An empty blocker set is rewritten to NemoClaw/tools/pr-review-advisor/analyze.mts Lines 682 to 707 in d9cb9f2 Consequences:
This does not directly merge or modify code—the repository policy still requires a human—but Required fix: Make positive advisor output advisory only. Require independent human/security-owner review for workflows, credentials, policies, installers, and sandbox boundaries. Exact security-category completeness should be enforced, and incomplete output should become Additional risks
Nine-category assessment
What the PR gets rightThe core architecture is substantially safer than the deleted E2E advisor:
So the immediate operational rule is: do not run advisor-suggested E2E commands containing Verification notesReviewed exact head
Files reviewedAll 37 PR files were inspected:
Supporting trust-boundary files and the exact Pi runtime implementation were also inspected, including |
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
<!-- markdownlint-disable MD041 --> ## Summary Replace roster-sensitive CLI coverage allocation with stable project/path ownership so adding, removing, or renaming one test no longer moves unrelated tests between coverage shards. The eight-shard parallel run remains intact, and the checked-in timing model keeps the slowest shard within 5% of average load. The first coverage comparison for this PR may show one final set of per-file changes because it intentionally moves from the old allocation to the stable baseline; subsequent test-roster changes will preserve existing ownership. ## Changes - Assign each CLI and integration test to a shard with a fixed SHA-256 hash of its project and repository-relative path. - Keep timing hints as an estimated-load measurement and fail a deterministic 1,344-entry balance fixture if its slowest shard exceeds 105% of average load. The actual current roster has a 933,721 ms estimated maximum versus the previous allocator's 898,605 ms maximum (+3.9%). Across three live CI runs, the slowest shard's wall time was 5m50s, 5m51s, and 6m14s; the final run's test command took 5m26s versus 5m22s on PR #6745 (+1.3%), while total job wall time was 6m14s versus 6m08s (+1.6%). - Add regression coverage proving insertions and removals do not move existing files, discovery order does not matter, fixed ownership remains recorded, and all files are assigned exactly once. - Record the escaped-defect boundary in the tests: the prior longest-processing-time allocator was deterministic for one roster but recomputed every ownership when that roster changed. Existing tests covered deterministic discovery order and balance, but not ownership stability across roster changes. ## Type of Change - [x] 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 <!-- Check exactly one tests line and one docs line. Check other lines when applicable. Add every requested justification or approval reference. --> - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [ ] Docs updated for user-facing behavior changes - [x] Docs not applicable — justification: internal CI/test-harness partitioning only; contributor documentation remains accurate - [ ] 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: ## Verification <!-- Check each applicable item only when supported by the requested evidence. Run targeted tests once per relevant change set and rerun after later edits or hook autofixes that can affect the tested behavior. Do not rerun hook-covered checks. --> - [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 — `npx vitest run --project integration test/cli-coverage-sequencer.test.ts` (8 tests passed) - [x] Applicable broad gate passed — `npm test` (1,468 files and 16,584 tests passed; 3 files and 40 tests skipped) - [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) --- <!-- DCO sign-off is required in this PR description, and every commit must appear as Verified in GitHub. Run: git config user.name && git config user.email --> Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/pr-review-advisor-workflow-boundary.test.ts (1)
434-486: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMock
sudoin the isolated command PATH.Line 481 restricts lookup to
binDir, but lines 439-460 do not createsudo. The workflow’ssudo apt-get install ...exits before the installation assertions run.Proposed fix
- for (const name of ["npm", "rm", "ln"]) writeFakeCommand(binDir, name); + for (const name of ["npm", "rm", "ln", "sudo"]) writeFakeCommand(binDir, name);🤖 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-workflow-boundary.test.ts` around lines 434 - 486, Update the isolated command setup in the test “installs and verifies the pinned search tools” so the mocked command set includes sudo alongside npm, rm, and ln. Ensure the fake sudo executable is available in binDir before spawning the workflow, allowing the existing installation assertions to run with PATH restricted to that directory.
🧹 Nitpick comments (2)
tools/pr-review-advisor/schema.json (1)
271-284: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWorkflow allow-list duplicated between schema and normalizer.
e2eTargetRecommendation.workflowhardcodes"e2e.yaml"here, whiletools/advisors/e2e-recommendations.mtsmaintains its ownALLOWED_WORKFLOWSset for the same purpose (used insanitizeTargetRecommendations, Line 633). Two independently-maintained sources for the same trusted-workflow inventory can silently drift.Consider generating this schema constant from the same constant used in
ALLOWED_WORKFLOWS(e.g., via a build step or shared JSON fragment) rather than hand-duplicating the value.As per path instructions for
tools/{advisors,pr-review-advisor}/**: "Derive inventories and limits from a canonical source where possible; flag duplicated lists that can silently drift."🤖 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/schema.json` around lines 271 - 284, Remove the independently hardcoded workflow value in the e2eTargetRecommendation schema and derive it from the canonical ALLOWED_WORKFLOWS used by sanitizeTargetRecommendations in e2e-recommendations.mts. Use a shared generated artifact or JSON fragment so schema validation and normalization consume the same workflow inventory, while preserving the existing e2e.yaml allow-list behavior.Source: Path instructions
test/e2e-recommendations.test.ts (1)
614-642: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest depends on the real, unmocked trusted workflow file.
This test overrides only the analyzed
e2eWorkflowText;readTrustedE2eWorkflowText()still reads the actual.github/workflows/e2e.yamlfrom disk. It currently passes because that file has nosteal-secretsjob, but the test's guarantee is coupled to live repo state rather than a hermetic fixture, so a future edit to the trusted workflow could silently change what this test actually proves.Consider adding a trusted-workflow override parameter (mirroring the existing analyzed-workflow override) so this regression test is fully isolated from the real file.
🤖 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-recommendations.test.ts` around lines 614 - 642, Add a trusted-workflow text override to the test setup or relevant helper, mirroring the existing e2eWorkflowText override and ensuring normalizeE2eTargetAdvisorResult uses it instead of readTrustedE2eWorkflowText(). Update the “rejects a free-standing job” test to provide an explicit trusted-workflow fixture that excludes steal-secrets, keeping the assertion independent of the repository’s live .github workflow.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.
Inline comments:
In @.agents/skills/nemoclaw-maintainer-day/SKILL.md:
- Line 32: Update the “Ready-now PR” criteria to require no unresolved
correctness or security issues, matching the threshold defined in MERGE-GATE.md,
rather than only excluding confirmed major issues. Preserve the existing CI,
conflict, and test requirements.
In `@tools/advisors/session.mts`:
- Around line 761-765: Wrap the credential setup calls in the credential
block—authStorage.setRuntimeApiKey and modelRegistry.registerProvider—in a
try/finally, and move deletion of process.env[credentialEnv] into the finally
block so it runs on both success and failure.
---
Outside diff comments:
In `@test/pr-review-advisor-workflow-boundary.test.ts`:
- Around line 434-486: Update the isolated command setup in the test “installs
and verifies the pinned search tools” so the mocked command set includes sudo
alongside npm, rm, and ln. Ensure the fake sudo executable is available in
binDir before spawning the workflow, allowing the existing installation
assertions to run with PATH restricted to that directory.
---
Nitpick comments:
In `@test/e2e-recommendations.test.ts`:
- Around line 614-642: Add a trusted-workflow text override to the test setup or
relevant helper, mirroring the existing e2eWorkflowText override and ensuring
normalizeE2eTargetAdvisorResult uses it instead of readTrustedE2eWorkflowText().
Update the “rejects a free-standing job” test to provide an explicit
trusted-workflow fixture that excludes steal-secrets, keeping the assertion
independent of the repository’s live .github workflow.
In `@tools/pr-review-advisor/schema.json`:
- Around line 271-284: Remove the independently hardcoded workflow value in the
e2eTargetRecommendation schema and derive it from the canonical
ALLOWED_WORKFLOWS used by sanitizeTargetRecommendations in
e2e-recommendations.mts. Use a shared generated artifact or JSON fragment so
schema validation and normalization consume the same workflow inventory, while
preserving the existing e2e.yaml allow-list behavior.
🪄 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: e0886a16-d67f-41e8-91c4-307238bdbbb1
📒 Files selected for processing (32)
.agents/skills/nemoclaw-maintainer-day/MERGE-GATE.md.agents/skills/nemoclaw-maintainer-day/PR-REVIEW-PRIORITIES.md.agents/skills/nemoclaw-maintainer-day/SKILL.md.agents/skills/nemoclaw-maintainer-day/scripts/check-gates.ts.agents/skills/nemoclaw-maintainer-day/scripts/pra-gate.ts.agents/skills/nemoclaw-maintainer-pr-comparator/SKILL.md.agents/skills/nemoclaw-maintainer-pr-comparator/checks/tier-0-gates.md.agents/skills/nemoclaw-maintainer-pr-comparator/scripts/collect-gates.sh.agents/skills/nemoclaw-maintainer-pr-comparator/scripts/render-verdict.py.agents/skills/nemoclaw-maintainer-pr-comparator/templates/verdict.md.github/workflows/pr-review-advisor.yaml.pre-commit-config.yamltest/e2e-recommendations.test.tstest/pr-comparator-render-verdict.test.tstest/pr-review-advisor-ledger-tools.test.tstest/pr-review-advisor-security-boundaries.test.tstest/pr-review-advisor-test-depth.test.tstest/pr-review-advisor-workflow-boundary.test.tstest/pr-review-advisor.test.tstest/pr-workflow-contract.test.tstest/skills/check-gates-compliance.test.tstest/skills/check-gates-pra.test.tstools/advisors/README.mdtools/advisors/e2e-recommendations.mtstools/advisors/github.mtstools/advisors/session.mtstools/pr-review-advisor/README.mdtools/pr-review-advisor/analyze.mtstools/pr-review-advisor/comment.mtstools/pr-review-advisor/schema.jsontools/pr-review-advisor/workflow-boundary.mtstsconfig.cli.json
💤 Files with no reviewable changes (5)
- .agents/skills/nemoclaw-maintainer-pr-comparator/templates/verdict.md
- .agents/skills/nemoclaw-maintainer-pr-comparator/scripts/render-verdict.py
- test/pr-workflow-contract.test.ts
- test/skills/check-gates-pra.test.ts
- .agents/skills/nemoclaw-maintainer-day/scripts/pra-gate.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- tools/advisors/README.md
- test/pr-review-advisor-test-depth.test.ts
- .github/workflows/pr-review-advisor.yaml
- tools/pr-review-advisor/README.md
- tools/pr-review-advisor/workflow-boundary.mts
- test/pr-review-advisor.test.ts
- tools/pr-review-advisor/analyze.mts
Signed-off-by: Carlos Villela <cvillela@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 `@test/maintainer-skills-policy.test.ts`:
- Around line 183-191: Update the test case “uses the merge gate's
unresolved-issue threshold for ready-now PRs” to also assert that MERGE-GATE.md
does not contain the legacy “no confirmed major CodeRabbit or PR Review Advisor
issues” wording, while preserving the existing threshold assertions for both
documents.
🪄 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: cb8b369c-4945-4f0a-bf9b-8ec4582b584b
📒 Files selected for processing (4)
.agents/skills/nemoclaw-maintainer-day/SKILL.mdtest/maintainer-skills-policy.test.tstest/pr-review-advisor-security-boundaries.test.tstools/advisors/session.mts
🚧 Files skipped from review as they are similar to previous changes (2)
- tools/advisors/session.mts
- .agents/skills/nemoclaw-maintainer-day/SKILL.md
wscurran
added
area: security
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
cjagwani
left a comment
cjagwani
left a comment
There was a problem hiding this comment.
Requesting one focused security-boundary fix on exact head a0f61009d33c9ed2d21fe7421035cd5a97e43e65. The conflicts are resolved correctly and the other previously reported workflow concerns are fixed.
[P1] Command-shaped model output still survives normalization, and model fields can suppress deterministic coverage. tools/advisors/e2e-recommendations.mts accepts arbitrary coverage workflow/job values and command-shaped reason text; it also derives de-duplication IDs from the model-provided job. tools/pr-review-advisor/comment.mts then publishes the reason unchanged.
At this exact head, a valid e2e-all selector with reason Run gh workflow run e2e.yaml --ref attacker now survives and is rendered. A forged coverage item with job: state-backup-restore, workflow: evil.yaml, and another attacker command also survives, suppresses the canonical deterministic state-backup-restore coverage item, and renders both commands in the bot comment. This violates #6736’s explicit requirement that command-shaped output cannot survive normalization and recreates the social-engineering path toward a privileged/secret-bearing dispatch.
Please reject the entire model item when any free-text E2E field is command-shaped; allowlist/canonicalize coverage workflow/job/script identities; derive deterministic-floor de-duplication only from trusted canonical identities (or prepend/overwrite deterministic items); and add regressions for both payloads asserting that no command is rendered.
The trusted checkout, token separation, credential scrubbing, exact-head E2E gate, fork/control-plane restrictions, bot-only comment provenance, fixed dependency pins, and removal of Advisor merge authority otherwise passed review.
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
|
@cjagwani Addressed the focused security-boundary review in verified commit
Local evidence on this exact commit: 84 focused integration tests, 260 broader advisor/gate tests, CLI typecheck, normal pre-commit/pre-push hooks, secret scans, test-size checks, and docs build all passed. Please re-review the exact head when convenient. @coderabbitai review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
git log --oneline -1 778ae188b2be140e6ccb75be3d2b8e5fdc699909 2>&1 || echo "not found"Length of output: 294
I'll kick off a fresh review of the current head to verify these changes in context, including whether the broader items from the review (provenance/gate compatibility with live metadata, ✅ Action performedReview finished.
|
cjagwani
left a comment
cjagwani
left a comment
There was a problem hiding this comment.
Requesting changes on exact head 778ae188b2be140e6ccb75be3d2b8e5fdc699909.
[P2] Keep legitimate PR-added credential-free selectors through the trusted publishing boundary.
The analyzer reads changed test source from the isolated exact-head checkout and intentionally accepts a newly tagged canonical selector (tools/advisors/e2e-recommendations.mts:341-380,449-459,531-550). The publisher then rebuilds its selector inventory only from the trusted base/workflow checkout (tools/pr-review-advisor/comment.mts:375-415) and silently drops that exact-head selector.
A focused normalize-to-publish reproduction produced required selectors cloud-onboard, credential-sanitization, security-posture, and new-credential-free-proof, but the rendered sticky comment omitted only new-credential-free-proof. This defeats #6736 acceptance that useful selector guidance reaches the primary comment, although it does not weaken the authoritative E2E gate.
Please carry provenance-bound evidence for newly validated exact-head selectors into the publisher, or safely revalidate the exact-head changed source there, and add an end-to-end normalize-to-publish regression for a newly added credential-free target.
The prior command-shaped model-guidance blocker is fixed on this head: canonical allowlisting and trusted reason replacement survive independent publisher validation, and 85/85 focused tests passed.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test/e2e-recommendations.test.ts (1)
50-60: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHardcoded control-plane id list risks silent drift from the canonical inventory.
E2E_CONTROL_PLANE_JOB_IDSduplicates the control-plane job ids as a literal set. If the canonical inventory (in the advisor source) adds/removes/renames a control-plane job, this test fixture won't automatically follow, and tests could keep passing against a stale list.Consider importing/deriving this set from the same canonical source the normalizer uses, if it's exported, rather than re-declaring it here.
🤖 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-recommendations.test.ts` around lines 50 - 60, The E2E test hardcodes control-plane job IDs instead of using the canonical inventory. Replace E2E_CONTROL_PLANE_JOB_IDS in withoutControlPlaneRecommendations with the exported control-plane ID set or derive it from the same advisor source used by the normalizer, preserving the existing filtering behavior and avoiding a duplicate literal list.
🤖 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 `@test/pr-review-advisor-comment-cli.test.ts`:
- Around line 17-71: Extend the test around readCommentArtifacts to cover its
successful artifact-reading path, using the existing defaultSummary fixture and
a corresponding valid result fixture as needed. Assert that the returned
artifacts contain the expected summary and result data, while preserving the
existing missing-summary and missing-result error assertions.
In `@test/pr-review-advisor-security-boundaries.test.ts`:
- Around line 262-266: Strengthen the leakage assertion in the test around
buildComment by checking decoded output or distinctive payload fragments such as
“id_rsa” and “attacker.example”, in addition to the existing raw command
assertion. Ensure escaped command content cannot satisfy the test while
preserving the current expected command and workflow assertions.
---
Nitpick comments:
In `@test/e2e-recommendations.test.ts`:
- Around line 50-60: The E2E test hardcodes control-plane job IDs instead of
using the canonical inventory. Replace E2E_CONTROL_PLANE_JOB_IDS in
withoutControlPlaneRecommendations with the exported control-plane ID set or
derive it from the same advisor source used by the normalizer, preserving the
existing filtering behavior and avoiding a duplicate literal list.
🪄 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: 52acad78-6f5e-4084-b9f5-13c0c78393c5
📒 Files selected for processing (30)
.agents/skills/nemoclaw-maintainer-day/MERGE-GATE.md.agents/skills/nemoclaw-maintainer-day/PR-REVIEW-PRIORITIES.md.agents/skills/nemoclaw-maintainer-day/SKILL.md.agents/skills/nemoclaw-maintainer-day/scripts/check-gates.ts.agents/skills/nemoclaw-maintainer-day/scripts/shared.ts.agents/skills/nemoclaw-maintainer-pr-comparator/scripts/collect-gates.sh.github/workflows/pr-e2e-gate.yamltest/e2e-recommendations.test.tstest/e2e/README.mdtest/e2e/docs/README.mdtest/e2e/support/e2e-workflow.test.tstest/maintainer-skills-policy.test.tstest/pr-e2e-gate-exceptions.test.tstest/pr-e2e-gate-workflow.test.tstest/pr-e2e-gate.test.tstest/pr-review-advisor-comment-cli.test.tstest/pr-review-advisor-security-boundaries.test.tstest/pr-review-advisor.test.tstest/pr-risk-plan.test.tstest/skills/check-gates-compliance.test.tstest/skills/triage-runtime.test.tstools/advisors/e2e-recommendations.mtstools/advisors/e2e-text.mtstools/advisors/risk-plan.mtstools/e2e/pr-e2e-gate.mtstools/e2e/workflow-boundary.mtstools/pr-review-advisor/README.mdtools/pr-review-advisor/analyze.mtstools/pr-review-advisor/comment.mtstools/pr-review-advisor/schema.json
🚧 Files skipped from review as they are similar to previous changes (23)
- test/skills/triage-runtime.test.ts
- .agents/skills/nemoclaw-maintainer-day/scripts/shared.ts
- .agents/skills/nemoclaw-maintainer-pr-comparator/scripts/collect-gates.sh
- test/e2e/support/e2e-workflow.test.ts
- .agents/skills/nemoclaw-maintainer-day/SKILL.md
- test/maintainer-skills-policy.test.ts
- test/e2e/docs/README.md
- tools/pr-review-advisor/schema.json
- tools/pr-review-advisor/README.md
- .github/workflows/pr-e2e-gate.yaml
- test/pr-risk-plan.test.ts
- tools/e2e/workflow-boundary.mts
- tools/advisors/risk-plan.mts
- test/pr-e2e-gate-workflow.test.ts
- .agents/skills/nemoclaw-maintainer-day/scripts/check-gates.ts
- .agents/skills/nemoclaw-maintainer-day/PR-REVIEW-PRIORITIES.md
- tools/pr-review-advisor/comment.mts
- tools/advisors/e2e-recommendations.mts
- test/skills/check-gates-compliance.test.ts
- tools/e2e/pr-e2e-gate.mts
- test/pr-e2e-gate.test.ts
- tools/pr-review-advisor/analyze.mts
- test/pr-review-advisor.test.ts
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
|
Addressed the exact-head credential-free selector publication gap in
Verification on this head: 15 advisor/gate test files, 259 tests passed; CLI and checked-JavaScript typechecks passed; @cjagwani please re-review this exact head when convenient. @coderabbitai review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
git log --oneline -1 5e6ab1010238a0d52ed4a7037b67e482d9deac0b 2>&1 || echo "not found"
git show --stat 5e6ab1010238a0d52ed4a7037b67e482d9deac0b 2>&1 | head -50Length of output: 1313
I'll kick off a fresh review of this head to verify the fix in context and check whether the remaining broader items from the independent review (live-metadata provenance/gate compatibility, ✅ Action performedReview finished.
|
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
## Summary This follow-up to #6745 binds `E2E / PR Gate` to the exact pull request number, head SHA, and base SHA, so base retargets and stale workflow events cannot reuse or overwrite another diff's result. Unrelated `main` advances no longer force a full CI rerun when GitHub proves the trusted E2E control plane is unchanged; ambiguous or changed trust boundaries still fail closed. ## Changes - Give canonical PR CI a strict run identity and require the trusted `.github/workflows/pr.yaml` path before the privileged coordinator accepts it. - Isolate metadata-only edits from eligible CI/advisor concurrency while rerunning both advisors for real base retargets. - Bind check external IDs, controller state, child dispatch inputs, evidence finalization, and manual exceptions to both head and base SHAs; serialize seed/coordinator mutations and revalidate the live open PR before every security-sensitive transition. - Pass the accepted workflow SHA to the child E2E workflow and reject a dispatch race before matrix generation or secret-bearing jobs. - Accept a newer `main` only when GitHub proves it is a stable descendant with fewer than 300 fully enumerated changed files and no `e2e-control-plane` match. This compatibility path is required by the trusted workflow-run controller because exact tip equality made unrelated merges invalidate completed PR CI; `test/pr-e2e-gate.test.ts` covers safe advances, control-plane paths and renames, divergence, truncation, and second advances. - Add regression coverage for out-of-order base events, A→B→A retargets, failed-CI provenance, closed/retargeted finalization, late manual-exception races, workflow source identity, and concurrency cancellation boundaries. Update the internal E2E and maintainer contracts. - Preserve the packaged `fdfind` fix from #6785 without duplicating it in this diff. ## Type of Change - [x] 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 - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [ ] Docs updated for user-facing behavior changes - [x] Docs not applicable — justification: This changes internal CI and maintainer behavior only; the internal E2E and merge-gate contracts were updated. - [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 correctness and security review covered workflow-event provenance, cancellation domains, exact-diff check identity, compare completeness, and race windows; every finding was addressed with a regression test. - [ ] 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 — `npx vitest run --project integration test/pr-e2e-gate.test.ts test/pr-e2e-gate-lifecycle.test.ts test/pr-e2e-gate-exceptions.test.ts test/pr-e2e-gate-workflow.test.ts test/pr-review-advisor-workflow-boundary.test.ts test/pr-workflow-contract.test.ts` (104 passed); `npx vitest run --project e2e-support test/e2e/support/e2e-operations-workflow-boundary.test.ts` (23 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 E2E gates now track exact head and base revisions, preventing results from being applied after retargeting or new commits. * Added secure workflow and base revision validation before E2E jobs are dispatched. * Added controlled manual resolution paths for eligible no-secret exceptions. * **Bug Fixes** * Improved handling of edited and retargeted pull requests. * Prevented stale or mismatched CI results from completing gates. * Strengthened cancellation and failure handling for incomplete or invalid E2E runs. * **Documentation** * Updated maintainer guidance for gate exceptions, ruleset rollouts, and validation requirements. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Carlos Villela <cvillela@nvidia.com>
<!-- markdownlint-disable MD041 --> ## Summary <!-- 1-3 plain sentences: what changes and why. Describe before-and-after behavior when it applies. Use existing repository terms; do not invent a label for this PR. --> This follow-up to #6745 and #6788 makes the combined PR Review Advisor disclose both model lanes without giving Nemotron authority over GPT's assessment, fixes contradictory confidence and E2E guidance, and ensures stale CI events close only the seeded in-progress E2E check. Missing or malformed Nemotron artifacts now degrade to an unavailable second opinion while a valid GPT review still publishes. ## Changes <!-- List concrete changes. If this adds an abstraction, configuration, fallback, migration, or compatibility path, name its current requirement and consumer, explain why a direct change is insufficient, and identify the test that protects it. --> - Download the exact same-run Nemotron artifact in the write-capable publisher, validate its raw status, final result, summary, size, path, and head SHA, and pass it to the comment renderer only through a trusted validation output. GPT remains fail-closed and authoritative; the nonblocking Nemotron lane reports only allowlisted status, confidence, severity counts, and opaque structural agreement or disagreement. - Render the actual normalized confidence for informational reviews, reconcile trusted E2E coverage and selector tiers through the existing target normalizer, and avoid “Why no…” text when optional guidance exists. - Delete only `github-actions[bot]` comments whose first line is one of the two retired E2E Advisor markers, after the combined sticky comment publishes. The bot-owner and exact-marker restrictions are covered by the security-boundary tests. - Recover an existing exact-diff check ID before stale workflow-run validation only when the check is still `in_progress`, allowing the existing cleanup step to terminalize abandoned checks without rewriting completed historical results. - Extend workflow-boundary, comment, normalization, GitHub API, and gate regression tests for complete, partial, failed, skipped, malformed, stale, and completed-check cases. ## Type of Change - [x] 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 <!-- Check exactly one tests line and one docs line. Check other lines when applicable. Add every requested justification or approval reference. --> - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [ ] Docs updated for user-facing behavior changes - [x] Docs not applicable — justification: This changes internal maintainer automation and GitHub workflow behavior only; the required documentation-writer pass found no user-facing CLI, configuration, API, policy, or product documentation change. - [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 final reviews found no remaining findings after checking `pull_request_target` privilege separation, same-run artifact provenance, primary fail-closed behavior, nonblocking secondary validation, prose isolation, exact bot-owned legacy comment deletion, E2E selector trust, and stale/completed check races. The nine-category security checklist also found no secret, injection, authorization, dependency, logging, cryptography, configuration, test, or holistic posture regression. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Verification <!-- Check each applicable item only when supported by the requested evidence. Run targeted tests once per relevant change set and rerun after later edits or hook autofixes that can affect that behavior. Do not rerun hook-covered checks. --> - [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 — final advisor suites passed 120/120 tests, the compacted advisor regression passed 44/44, and all four PR E2E gate suites passed 69/69; CLI type-check passed in the normal pre-push hook. - [ ] 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) --- <!-- DCO sign-off is required in this PR description, and every commit must appear as Verified in GitHub. Run: git config user.name && git config user.email --> 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 review comments now support an optional second-opinion analysis, including model-lane status and comparison details when compatible. * Comments show clearer confidence-aware model-lane information, with improved E2E “no coverage” messaging. * Legacy advisor sticky comments are cleaned up automatically during updates. * **Bug Fixes** * E2E recommendations/coverage handling is corrected when only optional coverage is selected. * E2E coverage/target reconciliation is more accurately aligned with available trusted capabilities. * PR gate handling is improved for stale, duplicate, and superseded checks—avoiding unnecessary check reopens and recording recovered check IDs. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
3 participants
Summary
Consolidates the standalone E2E Advisor into PR Review Advisor so each configured model lane opens one conversation that produces both code-review findings and E2E guidance. Automatic internal and fork reviews now use a trusted
pull_request_targetworkflow with read-only analysis, a separate write-capable publisher, and explicitly non-authoritative model output. GPT remains the primary comment lane and Nemotron remains an artifact-only second opinion.Related Issue
Closes #6736
Changes
${{ github.workflow_sha }}, fetch exact event base/head SHAs into an isolated data-only repository, remove symlinks, and keep model-bearing jobs read-only. Install and verify exactfd-find,ripgrep, Pi SDK, and TypeBox versions before secrets are exposed, then remove GitHub and model credentials from the process environment before model turns.merge_as_isand the PR Review Advisor maintainer/comparator gate. Advisor findings are untrusted review input for human adjudication; normalization always requires human review and enforces all nine canonical security categories.E2E / PR Gate.E2E / PR Gateon the exact PR head with deterministic trusted job selection, fail-closed evidence, no secret-bearing fork execution, and explicit no-secret exceptions for forks and E2E control-plane changes. Keep repository-ruleset activation blocked until the check has distinct provenance, strict up-to-date enforcement, and open-head backfill.Type of Change
Quality Gates
docs/page changes are needed.Verification
Verifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailablemain-syncnpm run test:changed(2,447/2,447); CLI build, root/CLI/plugin type checks, format checks, and test-size checks passed. Final command-boundary remediation also passed 84/84 focused tests, 260/260 broader advisor/gate tests, CLI typecheck, normal hooks and secret scans, plus the docs build with 0 errors (2 non-blocking Fern warnings).npm run checkpassed every pre-commit/manual hook except the CLI coverage ratchet under local Node 25, where all 14,571 tests passed but V8 mapped an unchanged file below its per-file floor. Re-running the failednpm run test:coverage:clihook with CI's installed Node 22.23.1 passed 1,305 files, 14,571 tests, and all coverage ratchets (74.94% lines, 67.93% branches, 76.59% functions, 73.91% statements).npm run docsbuilds without warnings (doc changes only)Signed-off-by: Carlos Villela cvillela@nvidia.com
Summary by CodeRabbit