Skip to content

fix: paginate pull request files in the dry run - #1081

Open
3rwww1 wants to merge 2 commits into
github-community-projects:main-enterprisefrom
3rwww1:fix/paginate-pull-request-files
Open

3rwww1 wants to merge 2 commits into
github-community-projects:main-enterprisefrom
3rwww1:fix/paginate-pull-request-files

Conversation

@3rwww1

@3rwww1 3rwww1 commented Sep 21, 2026

Copy link
Copy Markdown

Problem

  • A reviewer can merge a PR the dry run said changed nothing. The dry run (the check that previews config changes without applying them) hides part of the change, then the real sync applies it.
  • It only reads the first 30 changed files of a PR. pulls.listFiles is called without pagination, so the API default of 30 applies.
  • So a PR touching more than 30 files under .github/repos/ gets a partial preview. When no safe-settings config file lands on that first page, the check run says Number of repos that were considered: 0 and concludes success.
  • All three decisions in the handler read that same truncated list. The .github/settings.yml full-sync check, getChangedRepoConfigName and getChangedSubOrgConfigName all miss anything past page one.
  • Seen on an org with ~130 repos on 2.1.21. A 129-file PR reported 0 repos considered; a later 23-file push on the same PR reported the expected repos.

Fix

  • Paginate the call with per_page: 100. Five lines in index.js, and octokit.paginate is already used here for apps.listInstallations.
  • No behavior change for a PR under 30 files.

Test

  • No unit test added. test/unit/index.test.js is describe.skip('plugin', ...) in its entirety, so a new case there would never run, and none of its cases cover the check_run.created path.
  • Lint and tests pass. Pre-existing lint issue unchanged; 148 unit tests passed, 2 suites skipped.

Fixes #1080

@3rwww1

3rwww1 commented Sep 28, 2026

Copy link
Copy Markdown
Author

Hi @decyjphr, CI and CodeQL are green on this one. Gentle nudge, no rush, just making sure it is on your radar. Thanks for maintaining safe-settings!

@3rwww1
3rwww1 force-pushed the fix/paginate-pull-request-files branch from 1bd76e1 to 20f146d Compare October 14, 2026 00:01
Signed-off-by: Erwan Queffélec <airone.k@gmail.com>
@3rwww1

3rwww1 commented Oct 16, 2026

Copy link
Copy Markdown
Author

Rebased on main and fixed the unit test mock for the paginated file listing. Could a maintainer approve the workflow run so CI can check the new head? Thanks.

This branch has not been deployed

No deployments
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.

Dry run only inspects the first 30 changed files (pulls.listFiles is not paginated)

1 participant