From e8d49dc90a9b70a9ed148f68a55214979131c4c0 Mon Sep 17 00:00:00 2001 From: Rafa Leo Date: Tue, 18 Aug 2026 21:27:46 -0300 Subject: [PATCH 1/2] perf: skip archived repos without fetching them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `updateRepos` already skips archived repos (#991), but only after `archivePlugin.getState()` has spent a `repos.get` on each one. That call is avoidable: `GET /installation/repositories`, which `eachRepositoryRepos` already paginates, reports `archived` in its payload. Thread that flag through `checkAndProcessRepo` into `updateRepos` and skip before issuing any request. The saving is one API call per archived repo per full sync. In the organization where I found this, 2228 of 3013 repos (74%) are archived, so the majority of a full sync's rate-limit budget was spent fetching repos only to skip them — and on an installation the rate limit, not concurrency, is what bounds how long a full sync takes. The skip is conditional on the desired state, so an explicit `archived: false` in config is still processed: that is a request to unarchive. `getDesiredArchiveState()` reads config only and issues no request. Callers that do not know the archived state (single-repo webhook syncs) pass `undefined` and keep the existing behaviour, falling through to the `isArchived` check from #991. Four tests added: skip with no fetch when the caller reports archived, still fetch when config asks to unarchive, still fetch when the caller does not report the state, and the flag being threaded from the listing into `updateRepos`. Co-Authored-By: Claude AI-Assisted: yes AI-Tool: claude-code Co-Authored-By: claude-code --- lib/settings.js | 30 +++++++++++++++++--- test/unit/lib/settings.test.js | 50 ++++++++++++++++++++++++++++++++++ 2 files changed, 76 insertions(+), 4 deletions(-) diff --git a/lib/settings.js b/lib/settings.js index fd7ca2693..32e14ba70 100644 --- a/lib/settings.js +++ b/lib/settings.js @@ -362,6 +362,26 @@ ${this.results.reduce((x, y) => { if (overrideRepoConfig) { repoConfig = this.mergeDeep.mergeDeep({}, repoConfig, overrideRepoConfig) } + + // Archived repos are already skipped below, but only after + // archivePlugin.getState() has spent a repos.get on each one. When the caller + // knows the repo is archived — eachRepositoryRepos gets `archived` for free + // from GET /installation/repositories — skip before making any request. On a + // large org this is the difference between one wasted call per archived repo + // and none; orgs where most repos are archived spend the bulk of a full + // sync's rate-limit budget here. + // + // An explicit `archived: false` in config is a request to unarchive and must + // still be processed, so the desired state decides. getDesiredArchiveState() + // reads config only and issues no request. + if (repo.archived === true) { + const desiredArchiveState = new Archive(this.nop, this.github, repo, repoConfig, this.log).getDesiredArchiveState() + if (desiredArchiveState !== false) { + this.log.debug(`Skipping archived repo ${repo.repo} without fetching it`) + return + } + } + if (repoConfig) { try { this.log.debug(`found a matching repoconfig for this repo ${JSON.stringify(repoConfig)}`) @@ -540,18 +560,20 @@ ${this.results.reduce((x, y) => { log.debug('Fetching repositories') return github.paginate('GET /installation/repositories').then(repositories => { return Promise.all(repositories.map(repository => { - const { owner, name } = repository - return this.checkAndProcessRepo(owner.login, name) + // `archived` is already part of the listing payload, so passing it down + // lets updateRepos skip archived repos without spending an API call. + const { owner, name, archived } = repository + return this.checkAndProcessRepo(owner.login, name, archived) }) ) }) } - async checkAndProcessRepo (owner, name) { + async checkAndProcessRepo (owner, name, archived) { if (this.isRestricted(name)) { return null } - return this.updateRepos({ owner, repo: name }) + return this.updateRepos({ owner, repo: name, archived }) } /** diff --git a/test/unit/lib/settings.test.js b/test/unit/lib/settings.test.js index e102379a6..46bfed971 100644 --- a/test/unit/lib/settings.test.js +++ b/test/unit/lib/settings.test.js @@ -234,6 +234,56 @@ repository: }) }) }) // repoOverrideConfig + + describe('updateRepos with a known-archived repo', () => { + let settings + + beforeEach(() => { + stubConfig = { repository: { has_wiki: true }, restrictedRepos: { exclude: [] } } + // Built without a suborg on purpose: passing one sets subOrgConfigMap, and + // updateRepos then returns early for any repo outside that suborg. + settings = new Settings(false, stubContext, mockRepo, stubConfig, mockRef) + settings.subOrgConfigs = {} + settings.repoConfigs = {} + // repos.get is what archivePlugin.getState() calls. Asserting on it proves + // whether the archived repo was skipped before any request was made. + settings.github.rest.repos.get = jest.fn().mockResolvedValue({ data: { archived: true } }) + settings.github.rest.repos.update = jest.fn().mockResolvedValue({ data: {} }) + }) + + it('Skips without fetching the repo when the caller reports it archived', async () => { + await settings.updateRepos({ owner: 'test', repo: 'archived-repo', archived: true }) + expect(settings.github.rest.repos.get).not.toHaveBeenCalled() + }) + + it('Still processes the repo when config asks to unarchive it', async () => { + settings.config.repository.archived = false + await settings.updateRepos({ owner: 'test', repo: 'archived-repo', archived: true }) + expect(settings.github.rest.repos.get).toHaveBeenCalled() + }) + + it('Still processes the repo when the caller does not report archived state', async () => { + await settings.updateRepos({ owner: 'test', repo: 'some-repo' }) + expect(settings.github.rest.repos.get).toHaveBeenCalled() + }) + + it('Passes the archived flag from the repository listing through to updateRepos', async () => { + settings.github.paginate = jest.fn().mockResolvedValue([ + { name: 'active-repo', archived: false, owner: { login: 'test' } }, + { name: 'archived-repo', archived: true, owner: { login: 'test' } } + ]) + const seen = [] + settings.updateRepos = jest.fn(async (repo) => { seen.push(repo) }) + + await settings.eachRepositoryRepos(settings.github, settings.log) + + expect(seen).toEqual([ + { owner: 'test', repo: 'active-repo', archived: false }, + { owner: 'test', repo: 'archived-repo', archived: true } + ]) + }) + }) // updateRepos with a known-archived repo + describe('loadConfigs', () => { describe('load suborg configs', () => { beforeEach(() => { From 3d5c064f253f30d3b40cfbff5dabbaeb041317eb Mon Sep 17 00:00:00 2001 From: Rafa Leo Date: Tue, 18 Aug 2026 22:02:08 -0300 Subject: [PATCH 2/2] fix: keep archived off the repo ref and guard the no-config path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two review findings, both valid. Copilot: threading `archived` on the repo object leaks it into plugins. The Repository plugin does `Object.assign({}, settings, repo)` (plugins/repository.js:48) with `repo` last, and later `repos.update(this.settings)` (:215) — so an `archived` key on the ref lands in the update payload. In the unarchive flow that is destructive: the repo is unarchived, then the Repository plugin PATCHes `archived: true` back and re-archives it. `repos.get(this.repo)` (:67) would also receive a stray parameter. `archived` is now a separate argument to updateRepos and never touches the ref; plugins keep receiving a bare { owner, repo }. Second finding: the guard only covered the `repoConfig` branch. Without a repoConfig — a labels-only configuration, for instance — the else branch ran every child plugin with no archive check, so an archived repo still received forbidden writes. Added the same guard there, conditional on `archived !== false` so it costs nothing on the full-sync path: `false` from the listing needs no request, and `true` already returned earlier unless an unarchive was requested. Only a caller that does not know the state (single-repo webhook sync) pays one repos.get. Also dropped the duplicate Archive instantiation — the hoisted one is reused. Tests 141 -> 143. Two new: the no-repoConfig path skips child plugins for an archived repo, and `archived` does not appear on the ref passed to updateRepos. The threading test now asserts the argument position rather than a merged object. The labels stub is deliberately complete (endpoint.merge + paginate) so the no-write assertion fails loudly instead of passing because the plugin crashed. Co-Authored-By: Claude AI-Assisted: yes AI-Tool: claude-code Co-Authored-By: claude-code --- lib/settings.js | 46 +++++++++++++++++++++----------- test/unit/lib/settings.test.js | 48 ++++++++++++++++++++++++++++------ 2 files changed, 71 insertions(+), 23 deletions(-) diff --git a/lib/settings.js b/lib/settings.js index 32e14ba70..db6b4ad9f 100644 --- a/lib/settings.js +++ b/lib/settings.js @@ -328,7 +328,7 @@ ${this.results.reduce((x, y) => { } } - async updateRepos (repo) { + async updateRepos (repo, archived) { this.subOrgConfigs = this.subOrgConfigs || await this.getSubOrgConfigs() // Create a new object to avoid mutating the shared this.config.repository // This prevents race conditions when multiple repos are processed concurrently via Promise.all @@ -363,23 +363,21 @@ ${this.results.reduce((x, y) => { repoConfig = this.mergeDeep.mergeDeep({}, repoConfig, overrideRepoConfig) } - // Archived repos are already skipped below, but only after + const archivePlugin = new Archive(this.nop, this.github, repo, repoConfig, this.log) + + // Archived repos get skipped further down, but only after // archivePlugin.getState() has spent a repos.get on each one. When the caller - // knows the repo is archived — eachRepositoryRepos gets `archived` for free - // from GET /installation/repositories — skip before making any request. On a - // large org this is the difference between one wasted call per archived repo - // and none; orgs where most repos are archived spend the bulk of a full - // sync's rate-limit budget here. + // already knows — eachRepositoryRepos gets `archived` for free from + // GET /installation/repositories — skip before issuing any request. On orgs + // where most repos are archived this is where the bulk of a full sync's + // rate-limit budget goes. // // An explicit `archived: false` in config is a request to unarchive and must // still be processed, so the desired state decides. getDesiredArchiveState() // reads config only and issues no request. - if (repo.archived === true) { - const desiredArchiveState = new Archive(this.nop, this.github, repo, repoConfig, this.log).getDesiredArchiveState() - if (desiredArchiveState !== false) { - this.log.debug(`Skipping archived repo ${repo.repo} without fetching it`) - return - } + if (archived === true && archivePlugin.getDesiredArchiveState() !== false) { + this.log.debug(`Skipping archived repo ${repo.repo} without fetching it`) + return } if (repoConfig) { @@ -389,7 +387,6 @@ ${this.results.reduce((x, y) => { const childPlugins = this.childPluginsList(repo) const RepoPlugin = Settings.PLUGINS.repository - const archivePlugin = new Archive(this.nop, this.github, repo, repoConfig, this.log) const { isArchived, shouldArchive, shouldUnarchive } = await archivePlugin.getState() if (shouldUnarchive) { @@ -430,6 +427,20 @@ ${this.results.reduce((x, y) => { } } else { this.log.debug(`Didnt find any a matching repoconfig for this repo ${JSON.stringify(repo)} in ${JSON.stringify(this.repoConfigs)}`) + + // This branch has no repoConfig, so getState() was never reached and the + // isArchived guard above did not run — child plugins would still issue + // forbidden writes against an archived repo. Only verify when the archived + // state is not already known: `false` from the listing needs no request, + // and `true` already returned above unless an unarchive was requested. + if (archived !== false) { + const { isArchived, shouldUnarchive } = await archivePlugin.getState() + if (isArchived && !shouldUnarchive) { + this.log.debug(`Skipping child plugin updates for archived repo ${repo.repo}`) + return + } + } + const childPlugins = this.childPluginsList(repo) return Promise.all(childPlugins.map(([Plugin, config]) => { return new Plugin(this.nop, this.github, repo, config, this.log, this.errors).sync().then(res => { @@ -573,7 +584,12 @@ ${this.results.reduce((x, y) => { if (this.isRestricted(name)) { return null } - return this.updateRepos({ owner, repo: name, archived }) + // `archived` travels as its own argument, never merged into the repo ref: the + // Repository plugin does Object.assign({}, settings, repo) and later + // repos.update(settings), so an `archived` key on the ref would land in the + // update payload — and would re-archive a repo in the middle of unarchiving + // it. Plugins must keep receiving a bare { owner, repo }. + return this.updateRepos({ owner, repo: name }, archived) } /** diff --git a/test/unit/lib/settings.test.js b/test/unit/lib/settings.test.js index 46bfed971..51c2940d3 100644 --- a/test/unit/lib/settings.test.js +++ b/test/unit/lib/settings.test.js @@ -252,13 +252,13 @@ repository: }) it('Skips without fetching the repo when the caller reports it archived', async () => { - await settings.updateRepos({ owner: 'test', repo: 'archived-repo', archived: true }) + await settings.updateRepos({ owner: 'test', repo: 'archived-repo' }, true) expect(settings.github.rest.repos.get).not.toHaveBeenCalled() }) it('Still processes the repo when config asks to unarchive it', async () => { settings.config.repository.archived = false - await settings.updateRepos({ owner: 'test', repo: 'archived-repo', archived: true }) + await settings.updateRepos({ owner: 'test', repo: 'archived-repo' }, true) expect(settings.github.rest.repos.get).toHaveBeenCalled() }) @@ -267,20 +267,52 @@ repository: expect(settings.github.rest.repos.get).toHaveBeenCalled() }) - it('Passes the archived flag from the repository listing through to updateRepos', async () => { + it('Guards the no-repoConfig path too, so child plugins do not write to an archived repo', async () => { + // Label-only config: repoConfig is absent, so getState() is never reached + // by the main branch and the child plugins would run unguarded. + settings.config = { restrictedRepos: { exclude: [] }, labels: [{ name: 'bug' }] } + // Realistic stub: the labels plugin builds options via + // listLabelsForRepo.endpoint.merge and then calls github.paginate. Stubbing + // both means the plugin would run cleanly if it were reached, so the + // assertion below fails loudly instead of passing by accident. + settings.github.rest.issues = { + listLabelsForRepo: Object.assign(jest.fn(), { endpoint: { merge: jest.fn(() => ({})) } }) + } + settings.github.paginate = jest.fn().mockResolvedValue([]) + + await settings.updateRepos({ owner: 'test', repo: 'archived-repo' }) + + expect(settings.github.rest.repos.get).toHaveBeenCalled() + expect(settings.github.paginate).not.toHaveBeenCalled() + }) + + it('Does not leak archived onto the repo ref handed to plugins', async () => { + // The Repository plugin does Object.assign({}, settings, repo) and later + // repos.update(settings): an `archived` key on the ref would land in the + // update payload and re-archive the repo mid-unarchive. settings.github.paginate = jest.fn().mockResolvedValue([ - { name: 'active-repo', archived: false, owner: { login: 'test' } }, { name: 'archived-repo', archived: true, owner: { login: 'test' } } ]) const seen = [] - settings.updateRepos = jest.fn(async (repo) => { seen.push(repo) }) + settings.updateRepos = jest.fn(async (repo, archived) => { seen.push({ repo, archived }) }) await settings.eachRepositoryRepos(settings.github, settings.log) - expect(seen).toEqual([ - { owner: 'test', repo: 'active-repo', archived: false }, - { owner: 'test', repo: 'archived-repo', archived: true } + expect(seen).toEqual([{ repo: { owner: 'test', repo: 'archived-repo' }, archived: true }]) + expect(seen[0].repo).not.toHaveProperty('archived') + }) + + it('Passes the archived flag from the repository listing as a separate argument', async () => { + settings.github.paginate = jest.fn().mockResolvedValue([ + { name: 'active-repo', archived: false, owner: { login: 'test' } }, + { name: 'archived-repo', archived: true, owner: { login: 'test' } } ]) + const seen = [] + settings.updateRepos = jest.fn(async (repo, archived) => { seen.push([repo.repo, archived]) }) + + await settings.eachRepositoryRepos(settings.github, settings.log) + + expect(seen).toEqual([['active-repo', false], ['archived-repo', true]]) }) }) // updateRepos with a known-archived repo