Enhancement: Harden Review Sync infrastructure before Phase 2 (Caching, CI Rollback, Testing)

[darshit2308]

Problem

As we prepare to implement more complex routing logic (Phase 2), the current review-sync script has a few structural vulnerabilities that need to be addressed to ensure scalability and reliability:

1. API Rate Limiting: The script currently calls getPermissionLevel for every approver sequentially. If a maintainer approves multiple PRs, the redundant API calls burn through the GitHub API budget guarded by RATE_LIMIT_FLOOR.

2. Silent Failures: If the sync encounters an error, it logs to the action runner but exits with a success code, hiding broken syncs from immediate view.

3. Wasted Reviewer Time (No CI Rollback): Currently, if a PR fails its required CI checks (formatting, linting), it remains in the active review queues (queue:committers, etc.). This can mislead reviewers into spending time on broken code.

4. Lack of Test Coverage: There is no automated workflow to ensure the routing math stays green when future contributors modify the script.

5. Logical Routing Bug: The current approval logic separates maintainers and committers too strictly. This creates a logical flaw where a PR approved by two maintainers could be blocked because it lacks a standard committer approval.

6. Missing Community Visibility: A persistent "Open to community review" label on all PRs across all phases to encourage community participation.

Solution

I propose a "Pre-Phase 2" infrastructure update to harden the script. This PR will include 5 atomic fixes:

1. Add API Caching: Implement a module-level Map<username, role> cache in permissions.js to store role data during the cron run, drastically reducing API spend.

2. Add Action Error Handling: Update index.js to use core.setFailed() if the error count is greater than zero, ensuring silent passes do not hide broken syncs.

3. Implement CI Failure Rollback: Add a helper to check the GitHub Checks API (github.rest.checks.listForRef). If a PR has a failing CI conclusion, the script will override all approvals and actively demote the PR back to queue:junior-committer.

4. Add CI Testing Workflow: Create .github/workflows/test-review-sync.yml to trigger npm test specifically on changes touching the .github/scripts/review-sync/ directory.

5. Semantic Approval Logic: Refactor the approval counters in permissions.js to use "coreApprovals" (combining all users with write access or above) and update the determineLabel gate to ensure maintainers inherently count toward the core requirement.

6. Global Community Label: Define the "open to community review" label in constants and update syncLabel to ensure it is permanently attached to every open PR alongside the standard queue labels.

If anyone want suggest some more enhancements, they are more then welcome

Alternatives

No response

[exploreriii]

could we also add an extra label for community review throughout all phases, for now

[github-actions]

@coderabbitai plan

[coderabbitai]

Coding Plan

Summary

• Implement module-level caching in permissions.js to reduce redundant API calls for the same reviewers across multiple PRs
• Introduce a coreApproval semantic that unifies admin/maintain/write roles, simplifying label determination logic and fixing the strict maintainer/committer separation
• Add CI-aware label routing via a new checks.js helper that demotes PRs with failing builds to the junior queue, preventing reviewer time waste
• Replace silent process.exitCode with core.setFailed() for proper workflow failure visibility, and establish a CI workflow to test the review-sync scripts on every relevant PR

Design Choices

Design Choice 1: CI Check Scope: Which CI checks should trigger rollback to junior-committer?

Options Considered:

1. Check all workflow runs and fail if any has conclusion !== 'success'/'neutral'/'skipped'
2. Check only checks that are required by branch protection rules

Chosen Option: 1

Rationale: Check all workflow runs for the PR head SHA. Required checks are harder to determine programmatically and most repos expect all CI to pass before merge. This is also simpler to implement and more conservative.

Design Choice 2: Test Execution Strategy: How should the test workflow run tests without npm/package.json?

Options Considered:

1. Run each test file individually via node .github/scripts/review-sync/tests/test-*.js
2. Create a simple test runner script that executes all test files

Chosen Option: 1

Rationale: Run test files individually with separate workflow steps. This provides clearer failure isolation in CI and follows the existing pattern of direct node execution.

💡 User Tips

Regenerate the plan with different choices with @coderabbitai <feedback>.

Implementation Steps

Phase 1: Permission Layer Improvements

This phase addresses API rate limiting through caching and refactors the approval counting to use semantic "core approvals" terminology. Both changes modify permissions.js and are logically coupled—the semantic refactor builds on the same function that will use caching.

Task 1: Add Role Caching to getPermissionLevel

Implement a module-level cache in permissions.js to store role lookups during each cron run, eliminating redundant API calls when the same reviewer approves multiple PRs.

• Add a module-level Map at the top of helpers/permissions.js to store <username, role> mappings
• Modify getPermissionLevel() to check the cache before making API calls
• Store successful API responses in the cache before returning
• Cache 404 results as 'none' to avoid repeated lookups for external contributors
• Add a clearRoleCache() export for testing purposes (allows resetting cache between test runs)

Task 2: Refactor Approval Counting to Use coreApproval Semantic

Combine admin, maintain, and write roles into a single "core approvals" counter to fix the logical separation issue and simplify the determineLabel conditions.

• Update countApprovals() in helpers/permissions.js to add a new coreApproval field that sums admin + maintain + write approvals
• Keep existing maintainerApproval counter (still needed for the "requires senior approval" check)
• Update determineLabel() in helpers/labels.js Stage 1 condition to use coreApproval >= 2 instead of maintainerApproval + writeApproval >= 2
• Update any comments explaining the approval tiers to reflect the new semantic

Task 3: Add Tests for Caching and Core Approval Logic

Extend the existing test suite to cover the new caching behavior and coreApproval counter.

• Add tests in tests/test-permissions.js verifying that repeated calls to getPermissionLevel() with the same username only trigger one API call (check mock.calls.permissionsChecked length)
• Add test verifying that cache returns correct role on subsequent calls
• Add test for clearRoleCache() functionality
• Add tests in tests/test-labels.js for determineLabel() using the new coreApproval field
• Add test verifying two maintainer approvals (coreApproval=2, maintainerApproval=2) correctly returns ready-to-merge

🤖 Prompt for AI agents

Implement permission layer improvements in `helpers/permissions.js` and
`helpers/labels.js`, and extend the test suite to cover the new behavior.

**Task 1.1 — Add Role Caching to `getPermissionLevel`
(`helpers/permissions.js`):**
- Add a module-level `Map` at the top of the file to store `<username, role>`
mappings
- Modify `getPermissionLevel()` to check the cache before making API calls;
return the cached value if present
- Store successful API responses in the cache before returning
- Cache 404 results as `'none'` to avoid repeated lookups for external
contributors
- Export a `clearRoleCache()` function that clears the map (for test isolation)

**Task 1.2 — Refactor Approval Counting to Use `coreApproval` Semantic
(`helpers/permissions.js` and `helpers/labels.js`):**
- In `countApprovals()` (`helpers/permissions.js`), add a new `coreApproval`
field that sums admin + maintain + write approvals
- Keep the existing `maintainerApproval` counter, as it is still required for
the "requires senior approval" check
- In `determineLabel()` (`helpers/labels.js`), update the Stage 1 condition to
use `coreApproval >= 2` instead of `maintainerApproval + writeApproval >= 2`
- Update inline comments explaining approval tiers to reflect the new
`coreApproval` semantic

**Task 1.3 — Add Tests (`tests/test-permissions.js` and
`tests/test-labels.js`):**
- In `tests/test-permissions.js`, add tests verifying that repeated calls to
`getPermissionLevel()` with the same username only trigger one API call (assert
`mock.calls.permissionsChecked` length equals 1)
- Add a test verifying the cache returns the correct role on subsequent calls
- Add a test for `clearRoleCache()` ensuring the cache is empty after calling it
- In `tests/test-labels.js`, add tests for `determineLabel()` using the new
`coreApproval` field
- Add a test verifying that two maintainer approvals (coreApproval=2,
maintainerApproval=2) correctly returns the ready-to-merge label

Phase 2: CI-Aware Label Determination

This phase implements the CI failure rollback feature, which demotes PRs with failing builds back to the junior-committer queue regardless of approval status.

Task 1: Add CI Status Checking Helper

Create a helper function to query the GitHub Checks API and determine if a PR's CI has any failures.

• Add a new checks.js file in helpers/ with a hasCIFailure(github, owner, repo, ref) function
• Use github.rest.checks.listForRef() to fetch check runs for the PR's head SHA
• Return true if any check run has conclusion of 'failure', 'cancelled', or 'timed_out'
• Handle pagination if needed (use github.paginate)
• Export the new module through helpers/index.js barrel

Task 2: Integrate CI Checks into Label Sync Flow

Modify the label sync flow to check CI status and override approvals when builds are failing.

• Update syncLabel() in helpers/labels.js to accept the PR's head SHA (available from pr.head.sha)
• Before calling countApprovals() and determineLabel(), call hasCIFailure()
• If CI is failing, skip approval checking and force the label to QUEUE_LABELS.JUNIOR
• Log when a PR is being demoted due to CI failure (e.g., "⚠ PR #N has failing CI, demoting to junior-committer")
• Update the syncLabel call in index.js to pass the head SHA

Task 3: Add Tests for CI Rollback Behavior

Add test coverage for the CI failure detection and rollback logic.

• Create tests/test-checks.js following the existing test file pattern
• Update createMockGithub() in test-utils.js to support mocking checks.listForRef responses
• Add tests verifying hasCIFailure() returns true for failure/cancelled/timed_out conclusions
• Add tests verifying hasCIFailure() returns false for success/neutral/skipped conclusions
• Add integration test in test-labels.js verifying syncLabel() forces junior-committer when CI is failing, regardless of approvals

🤖 Prompt for AI agents

Implement the CI-aware label determination feature by creating a new `checks.js`
helper, integrating it into the label sync flow, and adding full test coverage.

**Task 2.1 — Create `helpers/checks.js`:**
- Create a new `checks.js` file in `helpers/`
- Implement and export `hasCIFailure(github, owner, repo, ref)` that uses
`github.paginate` with `github.rest.checks.listForRef()` to fetch all check runs
for the given `ref` (PR head SHA)
- Return `true` if any check run has a `conclusion` of `'failure'`,
`'cancelled'`, or `'timed_out'`; otherwise return `false`
- Export `hasCIFailure` through the `helpers/index.js` barrel

**Task 2.2 — Integrate CI checks into `syncLabel()` (`helpers/labels.js` and
`index.js`):**
- Update the `syncLabel()` function signature to accept the PR's head SHA
- At the top of `syncLabel()`, before calling `countApprovals()` or
`determineLabel()`, call `hasCIFailure()`
- If `hasCIFailure()` returns `true`, skip approval checking, force the label to
`QUEUE_LABELS.JUNIOR`, and log `"⚠ PR `#N` has failing CI, demoting to
junior-committer"`
- Update the `syncLabel` call in `index.js` to pass `pr.head.sha` as the head
SHA argument

**Task 2.3 — Add Tests (`tests/test-checks.js`, `test-utils.js`, and
`tests/test-labels.js`):**
- Create `tests/test-checks.js` following the existing test file pattern
- Update `createMockGithub()` in `test-utils.js` to support mocking
`checks.listForRef` responses
- In `test-checks.js`, add tests verifying `hasCIFailure()` returns `true` for
`failure`, `cancelled`, and `timed_out` conclusions
- Add tests verifying `hasCIFailure()` returns `false` for `success`, `neutral`,
and `skipped` conclusions
- In `test-labels.js`, add an integration test verifying that `syncLabel()`
forces the junior-committer label when CI is failing, regardless of approval
count

Phase 3: Infrastructure Hardening

This phase improves operational reliability by ensuring workflow failures are properly surfaced and establishing automated testing for future changes.

Task 1: Update Error Handling to Use core.setFailed()

Replace the current silent error signaling with proper GitHub Actions failure reporting.

• In index.js, replace process.exitCode = 1 with core.setFailed() when errors > 0
• Include the error count in the failure message (e.g., core.setFailed(\Sync completed with ${errors} error(s)`)`)
• Ensure the summary is still printed before calling setFailed()

Task 2: Create Test Workflow for Review-Sync Scripts

Add a GitHub Actions workflow that runs the test suite when review-sync code changes.

• Create .github/workflows/test-review-sync.yml
• Trigger on pull_request when paths match .github/scripts/review-sync/**
• Also trigger on push to main/master for the same paths
• Add workflow_dispatch for manual runs
• Use actions/checkout to get the code
• Run each test file with separate run steps: node .github/scripts/review-sync/tests/test-permissions.js, node .github/scripts/review-sync/tests/test-labels.js, node .github/scripts/review-sync/tests/test-reviews.js
• Add a final step for test-checks.js once created in Task 2.3

🤖 Prompt for AI agents

Harden the review-sync infrastructure by replacing silent error signaling with
`core.setFailed()` and creating a CI workflow to run the test suite
automatically.

**Task 3.1 — Update Error Handling (`index.js`):**
- Locate the section in `index.js` that sets `process.exitCode = 1` when errors
> 0
- Replace it with a call to `core.setFailed()` using the message: `` `Sync
completed with ${errors} error(s)` ``
- Ensure the workflow summary is still printed/logged before `setFailed()` is
called

**Task 3.2 — Create `.github/workflows/test-review-sync.yml`:**
- Create a new workflow file at `.github/workflows/test-review-sync.yml`
- Configure triggers:
  - `pull_request` with a `paths` filter for `.github/scripts/review-sync/**`
  - `push` to `main` and `master` with the same `paths` filter
  - `workflow_dispatch` for manual runs
- Add a job with `actions/checkout` to retrieve the code
- Add separate `run` steps for each test file:
  - `node .github/scripts/review-sync/tests/test-permissions.js`
  - `node .github/scripts/review-sync/tests/test-labels.js`
  - `node .github/scripts/review-sync/tests/test-reviews.js`
  - `node .github/scripts/review-sync/tests/test-checks.js`

Research

The review-sync system lives under .github/scripts/review-sync/ with a main entry point (index.js) and helpers organized in a namespaced barrel (helpers/index.js). Key modules include permissions.js (role checking and approval counting), labels.js (label determination and sync logic), and reviews.js (PR review fetching). Tests exist in tests/ using a custom vanilla Node.js test runner with boolean-return assertions and a createMockGithub() factory for mocking the GitHub API. The primary workflow (review-sync.yml) runs on a 30-minute cron schedule and supports manual dispatch. No npm/package.json exists; scripts run directly via Node.js.

---

🚀 Next Steps

🤖 All AI agent prompts combined

Task: 1

Implement permission layer improvements in `helpers/permissions.js` and
`helpers/labels.js`, and extend the test suite to cover the new behavior.

**Task 1.1 — Add Role Caching to `getPermissionLevel`
(`helpers/permissions.js`):**
- Add a module-level `Map` at the top of the file to store `<username, role>`
mappings
- Modify `getPermissionLevel()` to check the cache before making API calls;
return the cached value if present
- Store successful API responses in the cache before returning
- Cache 404 results as `'none'` to avoid repeated lookups for external
contributors
- Export a `clearRoleCache()` function that clears the map (for test isolation)

**Task 1.2 — Refactor Approval Counting to Use `coreApproval` Semantic
(`helpers/permissions.js` and `helpers/labels.js`):**
- In `countApprovals()` (`helpers/permissions.js`), add a new `coreApproval`
field that sums admin + maintain + write approvals
- Keep the existing `maintainerApproval` counter, as it is still required for
the "requires senior approval" check
- In `determineLabel()` (`helpers/labels.js`), update the Stage 1 condition to
use `coreApproval >= 2` instead of `maintainerApproval + writeApproval >= 2`
- Update inline comments explaining approval tiers to reflect the new
`coreApproval` semantic

**Task 1.3 — Add Tests (`tests/test-permissions.js` and
`tests/test-labels.js`):**
- In `tests/test-permissions.js`, add tests verifying that repeated calls to
`getPermissionLevel()` with the same username only trigger one API call (assert
`mock.calls.permissionsChecked` length equals 1)
- Add a test verifying the cache returns the correct role on subsequent calls
- Add a test for `clearRoleCache()` ensuring the cache is empty after calling it
- In `tests/test-labels.js`, add tests for `determineLabel()` using the new
`coreApproval` field
- Add a test verifying that two maintainer approvals (coreApproval=2,
maintainerApproval=2) correctly returns the ready-to-merge label
===============================================================================

Task: 2

Implement the CI-aware label determination feature by creating a new `checks.js`
helper, integrating it into the label sync flow, and adding full test coverage.

**Task 2.1 — Create `helpers/checks.js`:**
- Create a new `checks.js` file in `helpers/`
- Implement and export `hasCIFailure(github, owner, repo, ref)` that uses
`github.paginate` with `github.rest.checks.listForRef()` to fetch all check runs
for the given `ref` (PR head SHA)
- Return `true` if any check run has a `conclusion` of `'failure'`,
`'cancelled'`, or `'timed_out'`; otherwise return `false`
- Export `hasCIFailure` through the `helpers/index.js` barrel

**Task 2.2 — Integrate CI checks into `syncLabel()` (`helpers/labels.js` and
`index.js`):**
- Update the `syncLabel()` function signature to accept the PR's head SHA
- At the top of `syncLabel()`, before calling `countApprovals()` or
`determineLabel()`, call `hasCIFailure()`
- If `hasCIFailure()` returns `true`, skip approval checking, force the label to
`QUEUE_LABELS.JUNIOR`, and log `"⚠ PR `#N` has failing CI, demoting to
junior-committer"`
- Update the `syncLabel` call in `index.js` to pass `pr.head.sha` as the head
SHA argument

**Task 2.3 — Add Tests (`tests/test-checks.js`, `test-utils.js`, and
`tests/test-labels.js`):**
- Create `tests/test-checks.js` following the existing test file pattern
- Update `createMockGithub()` in `test-utils.js` to support mocking
`checks.listForRef` responses
- In `test-checks.js`, add tests verifying `hasCIFailure()` returns `true` for
`failure`, `cancelled`, and `timed_out` conclusions
- Add tests verifying `hasCIFailure()` returns `false` for `success`, `neutral`,
and `skipped` conclusions
- In `test-labels.js`, add an integration test verifying that `syncLabel()`
forces the junior-committer label when CI is failing, regardless of approval
count
===============================================================================

Task: 3

Harden the review-sync infrastructure by replacing silent error signaling with
`core.setFailed()` and creating a CI workflow to run the test suite
automatically.

**Task 3.1 — Update Error Handling (`index.js`):**
- Locate the section in `index.js` that sets `process.exitCode = 1` when errors
> 0
- Replace it with a call to `core.setFailed()` using the message: `` `Sync
completed with ${errors} error(s)` ``
- Ensure the workflow summary is still printed/logged before `setFailed()` is
called

**Task 3.2 — Create `.github/workflows/test-review-sync.yml`:**
- Create a new workflow file at `.github/workflows/test-review-sync.yml`
- Configure triggers:
  - `pull_request` with a `paths` filter for `.github/scripts/review-sync/**`
  - `push` to `main` and `master` with the same `paths` filter
  - `workflow_dispatch` for manual runs
- Add a job with `actions/checkout` to retrieve the code
- Add separate `run` steps for each test file:
  - `node .github/scripts/review-sync/tests/test-permissions.js`
  - `node .github/scripts/review-sync/tests/test-labels.js`
  - `node .github/scripts/review-sync/tests/test-reviews.js`
  - `node .github/scripts/review-sync/tests/test-checks.js`

💡 Iterate on the plan with: @coderabbitai <feedback>

Example Feedback
- `@coderabbitai` You can skip phase 3. Add a simple unit test case for phase 2.
- `@coderabbitai` For design choice 1 go ahead with option 3 and replan.

---

💬 Have feedback or questions? Drop into our discord!

[darshit2308]

hi @exploreriii , sure, I have updated the issue , adding the "community review" as the 6th feature.

[exploreriii]

An idea for the future could be if a PR is ready to review but has checks failing it should be marked as eg status: incomplete or status invalid
This is a step before junior committers

Albeit sometimes we get weird PRs that do fail but this is expected given the context and we do need to review them

[darshit2308]

@exploreriii noted as a future phase idea , will track it as a separate issue once Phase 2 is stable. the edge case you mentioned (expected CI failures) makes it worth thinking through carefully before implementing.