Skip to content

fix(ci): deduplicate PR Review Advisor watch trigger - #10343

Closed
rsliter wants to merge 5 commits into
mainfrom
codex/fix-watch-trigger-dedup
Closed

fix(ci): deduplicate PR Review Advisor watch trigger#10343
rsliter wants to merge 5 commits into
mainfrom
codex/fix-watch-trigger-dedup

Conversation

@rsliter

@rsliter rsliter commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Summary

One Vitest watch trigger declares the PR Review Advisor workflow-boundary test twice. This change removes the duplicate declaration. The existing behavior assertion continues to verify that the workflow resolves to the boundary test once.

Related Issue

Part of #10153.

Changes

  • Remove the duplicate PR Review Advisor boundary-test declaration from the watch trigger.
  • Keep the existing behavior-focused workflow-resolution assertion.
  • Leave every other watch mapping unchanged.

E2E root cause: vitest-watch-trigger-map / synthetic PR merge / duplicate PR Review Advisor boundary expectation

Source runs:

Failed jobs:

  • cli-test-shards (10) (98056646055)
  • cli-test-shards (10) (98055854426)
  • cli-test-shards (10) (98058394183)

Signature: test/repository/vitest-watch-triggers.test.ts:334 expects the same PR Review Advisor workflow-boundary path twice, but the resolver returns one unique path.

Scope: one root cause.

Competing candidates:

Current main's source-shape policy from PR #10401 does not permit a direct-registry uniqueness assertion without a compatibility exception. The earlier proposed assertion was removed; the existing behavior assertion covers the workflow result.

This branch derives the correction from the three CI failures and current main. It does not transfer either competing commit.

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: the existing PR Review Advisor workflow-resolution assertion expects the boundary test once.
  • Tests not applicable — justification:
  • Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging)
  • Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification:
  • Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue:

Documentation Writer Review

  • Documentation writer subagent reviewed the completed changes
  • Result: no-docs-needed
  • Evidence: Reviewed the complete one-line effective diff at commit 744d7b291a1d59fbb86414817adc52e69177d664 against base 6c06d0c8a3d935bf6e6c10f1c6e5b1cfbe56eea8. The change removes only the duplicate raw PR Review Advisor boundary-test mapping. It changes no documentation, comment, diagnostic, test title, CLI, configuration contract, or user workflow. The existing behavior assertion remains intact. Focused integration validation passed 101 tests, npm run validate:pr passed, and git diff --check passed.
  • Agent: Codex documentation writer subagent (/root/docs_review_10270)

DGX Station Hardware Evidence

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

Verification

  • PR description includes a Signed-off-by: line and every commit appears as Verified in GitHub — b627cce7c, 4f1891a77, 90b9a3e9c, 243bbc6e2, and 744d7b291 are Verified.
  • 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 — npm run validate:pr passed against base 6c06d0c8a after the guarded branch refresh.
  • Targeted behavior tests pass for the current change set, or tests are marked not applicable above — command/result: /Users/rsliter/Projects/NemoClaw/node_modules/.bin/vitest run --project integration test/repository/vitest-watch-triggers.test.ts test/automation/pull-requests/growth-guardrails.test.ts passed 101/101 tests.
  • Applicable broad gate passed — npm test for broad runtime/test-harness changes; npm run check for repo-wide validation/coverage changes — command/result:
  • 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)

Signed-off-by: Rebecca Sliter 571084+rsliter@users.noreply.github.com

Signed-off-by: Rebecca Sliter <571084+rsliter@users.noreply.github.com>
@rsliter rsliter self-assigned this Aug 26, 2026
@copy-pr-bot

copy-pr-bot Bot commented Aug 26, 2026

Copy link
Copy Markdown

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.

@coderabbitai

coderabbitai Bot commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: ab8cfbe0-2fb3-4b8e-b94b-1e75acb1541c

📥 Commits

Reviewing files that changed from the base of the PR and between 8bbc5b9 and 90b9a3e.

📒 Files selected for processing (2)
  • test/helpers/vitest-watch-triggers.ts
  • test/repository/vitest-watch-triggers.test.ts
💤 Files with no reviewable changes (1)
  • test/helpers/vitest-watch-triggers.ts

Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review.


📝 Walkthrough

Walkthrough

The PR removes the PR Review Advisor boundary test from one watch trigger and adds a regression test that verifies matched test files are unique.

Changes

PR Review Advisor watch triggers

Layer / File(s) Summary
Deduplicate watch-trigger test mappings
test/helpers/vitest-watch-triggers.ts, test/repository/vitest-watch-triggers.test.ts
The workflow trigger no longer runs the boundary test. The regression test resolves the trigger and verifies that matched test files contain no duplicates.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 90b9a

This localized test-trigger cleanup removes a duplicate declaration and adds regression coverage without changing production behavior; no actionable merge-blocking risk remains after normal checks and review.

Suggested reviewers: aasthajh

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1…
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.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: deduplicating the PR Review Advisor watch trigger in CI.
Full details: Docstring Coverage

Explanation

No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1 files.

✨ 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 codex/fix-watch-trigger-dedup

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

Signed-off-by: Rebecca Sliter <571084+rsliter@users.noreply.github.com>
@rsliter
rsliter marked this pull request as ready for review August 26, 2026 04:48
@github-code-quality

github-code-quality Bot commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Code Coverage Overview

Languages: TypeScript

TypeScript / code-coverage/plugin

The overall line coverage in commit 744d7b2 in the codex/fix-watch-trig... branch remains at 96%, unchanged from commit 6c06d0c in the main branch.


Updated August 26, 2026 13:59 UTC

@rsliter

rsliter commented Aug 26, 2026

Copy link
Copy Markdown
Contributor Author

Validation result for commit 744d7b291a1d59fbb86414817adc52e69177d664: PASS.

  • The diagnostic commit b627cce7c8bccdf4394477f5ce12b1b3430206bd reproduced the raw duplicate declaration. The focused selection reported 65 passing and 5 failing tests, including the four existing synthetic-merge cases.
  • The current effective diff removes only the duplicate raw declaration. The existing behavior assertion still verifies that the PR Review Advisor workflow resolves to one boundary-test path. No other watch mapping changes.
  • Current main's PR fix(advisor): flag source-shape tests #10401 source-shape policy required removing the proposed direct-registry uniqueness test. The exact-head Advisor's Test Design finding was valid and is addressed. Its Code Reduction proposal for a stronger source-shape assertion was declined because that would violate the same policy.
  • /Users/rsliter/Projects/NemoClaw/node_modules/.bin/vitest run --project integration test/repository/vitest-watch-triggers.test.ts test/automation/pull-requests/growth-guardrails.test.ts passed 101/101 tests.
  • npm run validate:pr passed its pre-commit, commitlint, and pre-push checks against base 6c06d0c8a. All five PR commits appear as Verified on GitHub.
  • The independent documentation writer review found no documentation change requirement. Receipt: docs-review-head-sha: 744d7b291; policy blob: docs-review-agents-blob-sha: c7de0eb73c13df83dd97dfb6e3f664cac8d75305.
  • Fresh GitHub Static Checks pass after PR chore(tooling): format value stream analyzer #10402. CLI shard 7 timed out after 5 seconds in unchanged src/lib/inference/serving/host-local-vllm-selection.test.ts. The Advisor could not configure its inference endpoint, so it produced no findings. Neither failure is owned by this one-line mapping change or matches a checked-in retry signature.
  • Comparator disposition: PR fix(mcp): preserve Hermes credential revisions #10270 commit d7a0a6a5f corrected the expected result but left the duplicate source declaration. PR fix(e2e): build changed sandbox images locally #10263 commit 6b5759447 contains the same raw removal inside a broad E2E change. This PR independently derives the focused one-line correction from the three CI failures and transfers no competing commit.

This correction addresses a deterministic repository CI failure. It has no live E2E target, so I did not dispatch a duplicate live E2E run.

@coderabbitai

coderabbitai Bot commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

Signed-off-by: Rebecca Sliter <571084+rsliter@users.noreply.github.com>
@github-actions

Copy link
Copy Markdown
Contributor

PR review advisory complete for commit 243bbc6: read the full review. Read it before deciding whether to request changes, approve, or merge this PR.

All previous runs

@rsliter rsliter closed this Aug 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant