From 030ca6716ea079f4bae3d7b95548ca2b7ed2d695 Mon Sep 17 00:00:00 2001 From: Ralf Anton Beier Date: Thu, 30 Apr 2026 00:18:15 +0200 Subject: [PATCH] fix: rulesets empty-contexts + skip control-surface repos in config MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Two production issues found by today's smoke test ### Bug 1 — temper-ops itself fails configuration `/Users/r/git/pulseengine/temper-ops` issue #1 (auto-created by the bot during `repository.created`): ❌ Configuration failed: Upgrade to GitHub Pro or make this repository public to enable this feature. The bot tried to apply branch protection on the new private control- surface repo. Free-plan private repos can't have branch protection. More importantly, even on Pro, temper-ops shouldn't be configured at all — it's a meta repo (issue forms only), not org code. ### Bug 2 — rulesets API rejects empty `required_status_checks` `/sync-all-repos` logs across timedate-mcp, template-mcp-server, bazel-file-ops-component, …: Validation Failed: "Invalid rule 'required_status_checks': Invalid parameter required_status_checks: Expected at least 1 elements, got 0" Our config has `contexts: []`. Legacy branch protection accepts that; Rulesets API does not. Translator emitted a half-valid rule and the entire ruleset POST 422'd, silently falling back to legacy. Net effect: no rulesets actually being applied during the org sweep. ## Fixes `src/rulesets.js` — translator now skips the `required_status_checks` rule entirely when contexts is empty or missing. The rest of the ruleset ships fine. Users who want strict status checks must list at least one context name. `src/config.js` — new `isControlSurfaceRepo(fullName)` helper. Returns true when the name matches a configured-and-enabled `chatops_repo.repo` or `controller_repo.repo`. Used by: - `src/repository.js` `configureRepository` — early returns `{success: true, skipped: 'control-surface'}`. Avoids issue spam, branch-protection 403s, dependabot noise on a repo with no code. - `src/organization.js` `synchronizeAllRepositories` — skips with a log line, doesn't even attempt configureRepository. ## Test plan - [x] All 806 tests pass (was 799 — added 7: rulesets-empty-contexts + isControlSurfaceRepo enable/disable/match-cases) - [x] eslint clean - [ ] After deploy: re-trigger /sync-all-repos. Logs should NOT show "Rulesets not available" 422 fallbacks anymore. temper-ops should be skipped entirely (no failed-config issue, no branch-protection attempt). ## Risk & rollout - Risk: low. Both fixes are narrow; the rulesets translator change only affects the empty-contexts case (current default), and the control- surface skip only fires for the explicitly-named admin repos. - Rollout: self-update on merge. Then re-run /sync-all-repos via the ChatOps issue form to actually apply rulesets across the org this time. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.7 (1M context) --- __tests__/integration/rulesets.test.js | 14 ++++++++++ __tests__/unit/schema.test.js | 37 ++++++++++++++++++++++++++ src/config.js | 18 +++++++++++++ src/organization.js | 7 ++++- src/repository.js | 12 ++++++++- src/rulesets.js | 12 +++++++-- 6 files changed, 96 insertions(+), 4 deletions(-) diff --git a/__tests__/integration/rulesets.test.js b/__tests__/integration/rulesets.test.js index af6150b..2a1acb0 100644 --- a/__tests__/integration/rulesets.test.js +++ b/__tests__/integration/rulesets.test.js @@ -57,6 +57,20 @@ describe('translateBranchProtectionToRuleset', () => { expect(pr.parameters.required_review_thread_resolution).toBe(true); }); + it('omits required_status_checks rule entirely when contexts is empty (Rulesets API rejects empty)', () => { + const r = translateBranchProtectionToRuleset({ + required_status_checks: { strict: true, contexts: [] } + }); + expect(r.rules.find((x) => x.type === 'required_status_checks')).toBeUndefined(); + }); + + it('omits required_status_checks rule when contexts is missing', () => { + const r = translateBranchProtectionToRuleset({ + required_status_checks: { strict: true } + }); + expect(r.rules.find((x) => x.type === 'required_status_checks')).toBeUndefined(); + }); + it('emits required_status_checks with strict policy and contexts', () => { const r = translateBranchProtectionToRuleset({ required_status_checks: { strict: true, contexts: ['ci/build', 'ci/test'] } diff --git a/__tests__/unit/schema.test.js b/__tests__/unit/schema.test.js index 693f3b5..006c6be 100644 --- a/__tests__/unit/schema.test.js +++ b/__tests__/unit/schema.test.js @@ -1,4 +1,41 @@ import { validateConfig } from '../../src/schema.js'; +import { _setConfigForTesting, isControlSurfaceRepo } from '../../src/config.js'; + +describe('isControlSurfaceRepo', () => { + beforeEach(() => { _setConfigForTesting({}); }); + afterEach(() => { _setConfigForTesting({}); }); + + it('returns false when no chatops_repo / controller_repo configured', () => { + expect(isControlSurfaceRepo('pulseengine/temper')).toBe(false); + }); + + it('matches chatops_repo.repo when enabled', () => { + _setConfigForTesting({ + chatops_repo: { enabled: true, repo: 'pulseengine/temper-ops' } + }); + expect(isControlSurfaceRepo('pulseengine/temper-ops')).toBe(true); + expect(isControlSurfaceRepo('pulseengine/temper')).toBe(false); + }); + + it('does NOT match when chatops_repo is configured but disabled', () => { + _setConfigForTesting({ + chatops_repo: { enabled: false, repo: 'pulseengine/temper-ops' } + }); + expect(isControlSurfaceRepo('pulseengine/temper-ops')).toBe(false); + }); + + it('matches controller_repo.repo when enabled', () => { + _setConfigForTesting({ + controller_repo: { enabled: true, repo: 'pulseengine/repo-requests' } + }); + expect(isControlSurfaceRepo('pulseengine/repo-requests')).toBe(true); + }); + + it('handles non-string input safely', () => { + expect(isControlSurfaceRepo(undefined)).toBe(false); + expect(isControlSurfaceRepo(null)).toBe(false); + }); +}); describe('validateConfig', () => { it('accepts valid config', () => { diff --git a/src/config.js b/src/config.js index f562382..533978d 100644 --- a/src/config.js +++ b/src/config.js @@ -164,6 +164,24 @@ export function getChatopsRepoConfig() { return config?.chatops_repo || { enabled: false }; } +/** + * Returns true when `/` is a control-surface repo owned by + * Temper itself (the chatops admin repo or the issue-form controller). + * These repos are *not* org code; they're meta. They should be skipped by + * `configureRepository` and `synchronizeAllRepositories` to avoid: + * - branch-protection 403s (private repos on free plans) + * - dependabot/template noise on a repo with no code + * - the bot configuring itself into a corner + */ +export function isControlSurfaceRepo(fullName) { + if (typeof fullName !== 'string') return false; + const chatops = config?.chatops_repo; + const ctrl = config?.controller_repo; + if (chatops?.enabled && chatops?.repo === fullName) return true; + if (ctrl?.enabled && ctrl?.repo === fullName) return true; + return false; +} + export function getRequiredSignaturesFlag(protectionConfig = {}) { if (typeof protectionConfig.require_signed_commits === 'boolean') { return protectionConfig.require_signed_commits; diff --git a/src/organization.js b/src/organization.js index 3ed1354..8ce259c 100644 --- a/src/organization.js +++ b/src/organization.js @@ -1,4 +1,4 @@ -import { getTargetIssueLabels } from './config.js'; +import { getTargetIssueLabels, isControlSurfaceRepo } from './config.js'; import { getLogger } from './logger.js'; import { configureRepository } from './repository.js'; import { checkExistingDependabotConfig } from './dependabot.js'; @@ -37,6 +37,11 @@ async function synchronizeAllRepositories(octokit, org) { continue; } + if (isControlSurfaceRepo(repo.full_name)) { + getLogger().info(`Skipping control-surface repository: ${repo.full_name}`); + continue; + } + getLogger().info(`Processing repository: ${repo.full_name}`); const configResult = await configureRepository(octokit, repo); if (!configResult.success) { diff --git a/src/repository.js b/src/repository.js index 1f40d3e..289ff04 100644 --- a/src/repository.js +++ b/src/repository.js @@ -6,7 +6,8 @@ import { getBranchProtectionConfig, mergePullRequestRules, getTargetIssueLabels, - getRulesetConfig + getRulesetConfig, + isControlSurfaceRepo } from './config.js'; import { applyBranchProtection } from './branch-protection.js'; import { applyRulesetFromBranchProtection } from './rulesets.js'; @@ -79,8 +80,17 @@ async function configureRepository( const repoInfo = normalizeRepoInput(repoOrOwner, maybeRepo); const owner = repoInfo.owner.login; const repo = repoInfo.name; + const fullName = `${owner}/${repo}`; const defaultBranch = getDefaultBranch(repoInfo); + if (isControlSurfaceRepo(fullName)) { + getLogger().info( + { repo: fullName }, + 'Skipping configuration: control-surface repo (chatops_repo / controller_repo)' + ); + return { success: true, skipped: 'control-surface' }; + } + try { getLogger().info(`Configuring repository: ${owner}/${repo} (skipBranchScopedWork=${skipBranchScopedWork})`); diff --git a/src/rulesets.js b/src/rulesets.js index e0fe355..1aac029 100644 --- a/src/rulesets.js +++ b/src/rulesets.js @@ -53,12 +53,20 @@ export function translateBranchProtectionToRuleset(bpConfig = {}, name = TEMPER_ } const checks = bpConfig.required_status_checks; - if (checks && checks !== null) { + // The Rulesets API rejects `required_status_checks` rules with empty + // `required_status_checks` arrays ("Expected at least 1 elements, got 0"). + // Legacy branch-protection accepts `contexts: []`; rulesets do not. + // When the user has no specific named checks, omit the rule entirely — + // emitting a half-valid rule causes the entire ruleset POST to 422 and + // we silently fall back to legacy. Better to ship the ruleset without + // this rule than to ship no ruleset at all. + const hasContexts = checks && Array.isArray(checks.contexts) && checks.contexts.length > 0; + if (checks && checks !== null && hasContexts) { rules.push({ type: 'required_status_checks', parameters: { strict_required_status_checks_policy: checks.strict ?? false, - required_status_checks: (checks.contexts || []).map((c) => + required_status_checks: checks.contexts.map((c) => typeof c === 'string' ? { context: c } : c ) }