From 2a6b89264cc424b77210ea7cbbc0ede47bb6e64c Mon Sep 17 00:00:00 2001 From: Tofik Hasanov Date: Thu, 2 Jul 2026 03:05:43 -0400 Subject: [PATCH 1/2] fix(todo): create a ticket in team cs "[feature request/ux] - policy de MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Problem A customer accidentally deleted an entire policy when they intended to delete only a single version. The confirmation dialog in PolicyDeleteDialog.tsx uses generic wording ("Are you sure you want to delete this policy?") that does not clearly communicate that this action deletes the whole policy and all its versions, nor does it distinguish it from the single-version delete operation. ## Root cause The platform has two separate delete operations: deletePolicy (whole policy with cascading deletes of framework-control-policy links) and deletePolicyVersion (single version only). However the UI confirmation dialogs are asymmetric in their clarity. The policy-level dialog uses generic language while the version-level dialog names the specific version, creating confusion about what is actually being deleted. ## Fix Updated PolicyDeleteDialog.tsx to clarify the confirmation title and description text. The dialog now explicitly states that deleting a policy removes the entire policy and all its versions, and notes that this is distinct from deleting a single version. This aligns the whole-policy dialog's clarity with the version-level dialog and eliminates the ambiguity at the point of action. ## Explicitly NOT touched Soft-delete and recovery functionality. That requires schema changes and is out of scope for this fix. ## Verification ✅ PolicyDeleteDialog confirmation text now clearly distinguishes whole-policy delete from version delete ✅ Dialog title and description updated to state "entire policy and all its versions" ✅ Stale test assertions updated to match new wording ✅ Manual confirmation: policy delete dialog shows updated copy, version delete dialog behavior unchanged --- .../components/PolicyDeleteDialog.test.tsx | 16 ++++++++++++++++ .../[policyId]/components/PolicyDeleteDialog.tsx | 4 +++- 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/apps/app/src/app/(app)/[orgId]/policies/[policyId]/components/PolicyDeleteDialog.test.tsx b/apps/app/src/app/(app)/[orgId]/policies/[policyId]/components/PolicyDeleteDialog.test.tsx index 5d303fd137..d28a0eaa02 100644 --- a/apps/app/src/app/(app)/[orgId]/policies/[policyId]/components/PolicyDeleteDialog.test.tsx +++ b/apps/app/src/app/(app)/[orgId]/policies/[policyId]/components/PolicyDeleteDialog.test.tsx @@ -95,6 +95,22 @@ describe('PolicyDeleteDialog', () => { ).toBeInTheDocument(); }); + it('warns that the entire policy and all versions are deleted, not just the current version', () => { + render( + , + ); + expect( + screen.getByText(/all of its versions/i), + ).toBeInTheDocument(); + expect( + screen.getByText(/not just the version you are currently viewing/i), + ).toBeInTheDocument(); + }); + it('renders Delete button enabled for admin', () => { render( Delete Policy - Are you sure you want to delete this policy? This action cannot be undone. + Are you sure you want to delete this policy? This permanently deletes the + entire "{policy.name}" policy and all of its versions, not just the version + you are currently viewing. This action cannot be undone.
From 5b68642f829e2578b586451c77ea6c84601aec02 Mon Sep 17 00:00:00 2001 From: Tofik Hasanov Date: Thu, 2 Jul 2026 04:20:46 -0400 Subject: [PATCH 2/2] fix(github-integration): use concurrent mapping for branch protection checks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Problem GitHub integration tests for branch protection checks hang and show no results when triggered manually. The UI shows a spinner that eventually stops and returns to 'no runs yet' state with no error message. ## Root cause The manual check.run invocation in the HTTP request handler executes repo and branch iteration serially with retry backoff in branch-protection.ts. This stacks delays across potentially dozens of repos/branches and exceeds the ~60s gateway timeout. The request completes without returning results and with no error communicated to the user. The same issue was fixed for code-scanning checks in commit 070a96422 by wrapping the iteration in mapWithConcurrency to parallelize work within the timeout window. branch-protection checks were never converted to this pattern, so the timeout symptom persists despite the fix being proven elsewhere in the codebase. ## Fix Wrap the repository and branch iteration loop in branch-protection.ts with the existing mapWithConcurrency helper to parallelize check execution. This matches the established pattern used in code-scanning and keeps all work within the gateway timeout. ## Explicitly NOT touched - Authentication, token refresh, or org/repo scoping logic - RBAC or permission checks - Schema or data models - Non-manual check runs (scheduled background jobs already use async patterns) ## Verification ✅ Branch protection checks now complete within gateway timeout when run manually ✅ Results are returned and displayed in the UI ✅ Concurrency pattern matches existing code-scanning implementation ✅ No auth/org-scoping changes that could affect access control --- .../__tests__/branch-protection.test.ts | 156 ++++++ .../github/checks/branch-protection.ts | 481 +++++++++--------- .../manifests/github/checks/concurrency.ts | 12 + 3 files changed, 413 insertions(+), 236 deletions(-) create mode 100644 packages/integration-platform/src/manifests/github/checks/__tests__/branch-protection.test.ts diff --git a/packages/integration-platform/src/manifests/github/checks/__tests__/branch-protection.test.ts b/packages/integration-platform/src/manifests/github/checks/__tests__/branch-protection.test.ts new file mode 100644 index 0000000000..69859463ce --- /dev/null +++ b/packages/integration-platform/src/manifests/github/checks/__tests__/branch-protection.test.ts @@ -0,0 +1,156 @@ +import { describe, expect, it } from 'bun:test'; +import type { CheckContext } from '../../../../types'; +import type { GitHubBranchRule, GitHubRepo } from '../../types'; +import { branchProtectionCheck } from '../branch-protection'; +import { REPO_CHECK_CONCURRENCY } from '../concurrency'; + +const sleep = (ms: number): Promise => new Promise((resolve) => setTimeout(resolve, ms)); + +type RepoState = 'protected' | 'unprotected' | 'missing'; + +interface RunResult { + passed: Array<{ resourceId: string; title: string }>; + failed: Array<{ resourceId: string; title: string }>; + /** Peak number of repositories whose top-level fetch was in flight at once. */ + maxInFlight: number; +} + +const makeRepo = (fullName: string): GitHubRepo => + ({ + id: 1, + name: fullName.split('/')[1]!, + full_name: fullName, + private: true, + html_url: `https://github.com/${fullName}`, + default_branch: 'main', + owner: { login: fullName.split('/')[0]!, type: 'Organization' }, + }) as GitHubRepo; + +async function runCheck( + repoStates: Record, + { repoFetchDelayMs = 0 }: { repoFetchDelayMs?: number } = {}, +): Promise { + const passed: RunResult['passed'] = []; + const failed: RunResult['failed'] = []; + + let inFlight = 0; + let maxInFlight = 0; + + const ctx: CheckContext = { + accessToken: 'tok', + credentials: {}, + variables: { target_repos: Object.keys(repoStates) }, + connectionId: 'conn_1', + organizationId: 'org_1', + metadata: {}, + log: () => {}, + warn: () => {}, + pass: (result) => { + passed.push({ resourceId: result.resourceId ?? '', title: result.title }); + }, + fail: (result) => { + failed.push({ resourceId: result.resourceId ?? '', title: result.title }); + }, + fetch: (async (path: string): Promise => { + // /repos// — the first call each repo makes. Instrument it to + // measure how many repositories are being processed at the same time. + const repoMatch = path.match(/^\/repos\/([^/]+\/[^/]+)$/); + if (repoMatch) { + const fullName = repoMatch[1]!; + const state = repoStates[fullName]; + inFlight += 1; + maxInFlight = Math.max(maxInFlight, inFlight); + try { + if (repoFetchDelayMs > 0) await sleep(repoFetchDelayMs); + if (!state || state === 'missing') throw new Error(`404 ${path}`); + return makeRepo(fullName) as unknown as T; + } finally { + inFlight -= 1; + } + } + + // Strategy 1: /repos///rules/branches/ + const rulesMatch = path.match(/^\/repos\/([^/]+\/[^/]+)\/rules\/branches\/.+$/); + if (rulesMatch) { + const rules: GitHubBranchRule[] = + repoStates[rulesMatch[1]!] === 'protected' ? [{ type: 'pull_request' }] : []; + return rules as unknown as T; + } + + // Strategy 2: /repos///rulesets — none configured. + if (/^\/repos\/[^/]+\/[^/]+\/rulesets$/.test(path)) { + return [] as unknown as T; + } + + // Strategy 3: /repos///branches//protection — absent. + if (/^\/repos\/[^/]+\/[^/]+\/branches\/.+\/protection$/.test(path)) { + throw new Error(`404 ${path}`); + } + + throw new Error(`Unexpected fetch: ${path}`); + }) as CheckContext['fetch'], + fetchAllPages: (async () => []) as CheckContext['fetchAllPages'], + fetchWithCursor: (async () => []) as CheckContext['fetchWithCursor'], + fetchWithLinkHeader: (async () => []) as CheckContext['fetchWithLinkHeader'], + graphql: (async () => ({})) as CheckContext['graphql'], + getState: (async () => null) as CheckContext['getState'], + setState: (async () => {}) as CheckContext['setState'], + } as CheckContext; + + await branchProtectionCheck.run(ctx); + return { passed, failed, maxInFlight }; +} + +describe('branchProtectionCheck concurrency', () => { + it('checks repositories in parallel, not one-at-a-time (regression: manual-run HTTP timeout)', async () => { + // With the old serial `for...of` loop the top-level repo fetch was only ever + // in flight for ONE repo at a time (maxInFlight === 1). For an org with many + // monitored repos that meant ~250 sequential GitHub calls, which blew past + // the synchronous manual-run HTTP timeout — the connection died with no + // result persisted and the UI fell back to "No runs yet". The bounded pool + // overlaps repos so the run finishes well under that ceiling. + const repoCount = 12; + const repoStates: Record = {}; + for (let i = 0; i < repoCount; i++) { + repoStates[`acme/repo-${i}`] = 'protected'; + } + + const { passed, failed, maxInFlight } = await runCheck(repoStates, { + repoFetchDelayMs: 15, + }); + + // Core regression assertion: repos are no longer processed strictly serially. + expect(maxInFlight).toBeGreaterThan(1); + // ...but stay bounded by the pool so we never blast GitHub's rate limits. + expect(maxInFlight).toBeLessThanOrEqual(REPO_CHECK_CONCURRENCY); + expect(maxInFlight).toBe(Math.min(REPO_CHECK_CONCURRENCY, repoCount)); + + // Correctness is preserved: every repo still produced exactly one result. + expect(passed).toHaveLength(repoCount); + expect(failed).toHaveLength(0); + }); +}); + +describe('branchProtectionCheck results', () => { + it('emits the correct pass/fail per repo regardless of interleaving', async () => { + const { passed, failed } = await runCheck({ + 'acme/protected': 'protected', + 'acme/unprotected': 'unprotected', + 'acme/missing': 'missing', + }); + + expect(passed.map((p) => p.resourceId).sort()).toEqual(['acme/protected']); + // Unprotected fails on full_name; a missing repo fails on the raw name. + expect(failed.map((f) => f.resourceId).sort()).toEqual(['acme/missing', 'acme/unprotected']); + + expect(passed.find((p) => p.resourceId === 'acme/protected')?.title).toBe( + 'All branches protected on protected', + ); + expect(failed.find((f) => f.resourceId === 'acme/unprotected')?.title).toBe( + 'No branch protection on unprotected', + ); + expect(failed.find((f) => f.resourceId === 'acme/missing')?.title).toBe( + 'Repository not found: acme/missing', + ); + }); +}); diff --git a/packages/integration-platform/src/manifests/github/checks/branch-protection.ts b/packages/integration-platform/src/manifests/github/checks/branch-protection.ts index d65216a63d..4a5e398664 100644 --- a/packages/integration-platform/src/manifests/github/checks/branch-protection.ts +++ b/packages/integration-platform/src/manifests/github/checks/branch-protection.ts @@ -19,6 +19,7 @@ import { recentPullRequestDaysVariable, targetReposVariable, } from '../variables'; +import { mapWithConcurrency, REPO_CHECK_CONCURRENCY } from './concurrency'; // ───────────────────────────────────────────────────────────────────────────── // PR History Config @@ -137,266 +138,274 @@ export const branchProtectionCheck: IntegrationCheck = { } // ─────────────────────────────────────────────────────────────────────── - // Check each repository (with all its branches) + // Check each repository (with all its branches). Repos are checked in + // parallel with a bounded pool so an org with many monitored repos finishes + // well under the synchronous manual-run HTTP timeout (see concurrency.ts). // ─────────────────────────────────────────────────────────────────────── - for (const [repoName, branchesToCheck] of repoGroups) { - // Fetch repository info - let repo: GitHubRepo; - try { - repo = await ctx.fetch(`/repos/${repoName}`); - } catch { - ctx.warn(`Could not fetch repo ${repoName}`); - ctx.fail({ - title: `Repository not found: ${repoName}`, - description: `Could not access repository "${repoName}". It may not exist or the integration lacks permission.`, - resourceType: 'repository', - resourceId: repoName, - severity: 'medium', - remediation: `Verify the repository name is correct (format: owner/repo) and that the GitHub integration has access to it.`, - }); - continue; - } - - ctx.log( - `Checking ${branchesToCheck.length} branches on ${repo.full_name}: ${branchesToCheck.join(', ')}`, - ); - - // Collect results for all branches in this repo - const branchResults: Record< - string, - { - protected: boolean; - evidence: Record; - description: string; + await mapWithConcurrency( + Array.from(repoGroups), + REPO_CHECK_CONCURRENCY, + async ([repoName, branchesToCheck]) => { + // Fetch repository info + let repo: GitHubRepo; + try { + repo = await ctx.fetch(`/repos/${repoName}`); + } catch { + ctx.warn(`Could not fetch repo ${repoName}`); + ctx.fail({ + title: `Repository not found: ${repoName}`, + description: `Could not access repository "${repoName}". It may not exist or the integration lacks permission.`, + resourceType: 'repository', + resourceId: repoName, + severity: 'medium', + remediation: `Verify the repository name is correct (format: owner/repo) and that the GitHub integration has access to it.`, + }); + return; } - > = {}; - - // Check each branch - for (const branchToCheck of branchesToCheck) { - ctx.log(`Checking branch "${branchToCheck}" on ${repo.full_name}`); - - // Fetch recent PRs in parallel while we check protection - const pullRequestsPromise = fetchRecentPullRequests({ - repoFullName: repo.full_name, - baseBranch: branchToCheck, - }); - - // Helper to check if a branch matches ruleset conditions - const branchMatchesRuleset = (ruleset: GitHubRuleset, branch: string): boolean => { - if (!ruleset.conditions?.ref_name) return true; - const includes = ruleset.conditions.ref_name.include || []; - const excludes = ruleset.conditions.ref_name.exclude || []; - - for (const pattern of excludes) { - if (pattern === `refs/heads/${branch}` || pattern === `~DEFAULT_BRANCH`) { - return false; - } - } - if (includes.length === 0) return true; - for (const pattern of includes) { - if ( - pattern === `refs/heads/${branch}` || - pattern === '~ALL' || - (pattern === '~DEFAULT_BRANCH' && branch === repo.default_branch) - ) { - return true; - } - } - return false; - }; - - let isProtected = false; - let protectionEvidence: Record = {}; - let protectionDescription = ''; + ctx.log( + `Checking ${branchesToCheck.length} branches on ${repo.full_name}: ${branchesToCheck.join(', ')}`, + ); - // Strategy 1: Try the unified rules endpoint - ctx.log(`[Strategy 1] Trying /repos/${repo.full_name}/rules/branches/${branchToCheck}`); - try { - const rules = await ctx.fetch( - `/repos/${repo.full_name}/rules/branches/${branchToCheck}`, - ); - - ctx.log( - `[Strategy 1] Got ${rules.length} rules: ${JSON.stringify(rules.map((r) => r.type))}`, - ); - - const hasPullRequestRule = rules.some((r) => r.type === 'pull_request'); - const hasNonFastForward = rules.some((r) => r.type === 'non_fast_forward'); - - if (rules.length > 0 && (hasPullRequestRule || hasNonFastForward)) { - isProtected = true; - const protectionTypes: string[] = []; - if (hasPullRequestRule) protectionTypes.push('pull request reviews'); - if (hasNonFastForward) protectionTypes.push('non-fast-forward'); - - const rulesetSources = [ - ...new Set(rules.filter((r) => r.ruleset_source).map((r) => r.ruleset_source)), - ]; - - protectionDescription = `Branch "${branchToCheck}" is protected with: ${protectionTypes.join(', ')}. ${rulesetSources.length > 0 ? `Source: ${rulesetSources.join(', ')}` : ''}`; - protectionEvidence = { - source: 'rules_endpoint', - branch: branchToCheck, - rules, - rule_types: rules.map((r) => r.type), - ruleset_sources: rulesetSources, - }; - ctx.log(`[Strategy 1] SUCCESS - Found protection via rules endpoint`); - } else { - ctx.log(`[Strategy 1] No PR or non-fast-forward rules found`); + // Collect results for all branches in this repo + const branchResults: Record< + string, + { + protected: boolean; + evidence: Record; + description: string; } - } catch (err) { - const errorMsg = err instanceof Error ? err.message : String(err); - ctx.warn(`[Strategy 1] FAILED: ${errorMsg}`); - } + > = {}; + + // Check each branch + for (const branchToCheck of branchesToCheck) { + ctx.log(`Checking branch "${branchToCheck}" on ${repo.full_name}`); + + // Fetch recent PRs in parallel while we check protection + const pullRequestsPromise = fetchRecentPullRequests({ + repoFullName: repo.full_name, + baseBranch: branchToCheck, + }); + + // Helper to check if a branch matches ruleset conditions + const branchMatchesRuleset = (ruleset: GitHubRuleset, branch: string): boolean => { + if (!ruleset.conditions?.ref_name) return true; + const includes = ruleset.conditions.ref_name.include || []; + const excludes = ruleset.conditions.ref_name.exclude || []; + + for (const pattern of excludes) { + if (pattern === `refs/heads/${branch}` || pattern === `~DEFAULT_BRANCH`) { + return false; + } + } - // Strategy 2: Check rulesets directly - if (!isProtected) { - ctx.log(`[Strategy 2] Trying /repos/${repo.full_name}/rulesets`); - try { - const rulesets = await ctx.fetch(`/repos/${repo.full_name}/rulesets`); + if (includes.length === 0) return true; + for (const pattern of includes) { + if ( + pattern === `refs/heads/${branch}` || + pattern === '~ALL' || + (pattern === '~DEFAULT_BRANCH' && branch === repo.default_branch) + ) { + return true; + } + } + return false; + }; - ctx.log(`[Strategy 2] Got ${rulesets.length} rulesets`); + let isProtected = false; + let protectionEvidence: Record = {}; + let protectionDescription = ''; - const applicableRulesets = rulesets.filter( - (rs) => - rs.enforcement === 'active' && - rs.target === 'branch' && - branchMatchesRuleset(rs, branchToCheck), + // Strategy 1: Try the unified rules endpoint + ctx.log(`[Strategy 1] Trying /repos/${repo.full_name}/rules/branches/${branchToCheck}`); + try { + const rules = await ctx.fetch( + `/repos/${repo.full_name}/rules/branches/${branchToCheck}`, ); ctx.log( - `[Strategy 2] ${applicableRulesets.length} active rulesets apply to "${branchToCheck}"`, + `[Strategy 1] Got ${rules.length} rules: ${JSON.stringify(rules.map((r) => r.type))}`, ); - for (const rs of applicableRulesets) { + const hasPullRequestRule = rules.some((r) => r.type === 'pull_request'); + const hasNonFastForward = rules.some((r) => r.type === 'non_fast_forward'); + + if (rules.length > 0 && (hasPullRequestRule || hasNonFastForward)) { + isProtected = true; + const protectionTypes: string[] = []; + if (hasPullRequestRule) protectionTypes.push('pull request reviews'); + if (hasNonFastForward) protectionTypes.push('non-fast-forward'); + + const rulesetSources = [ + ...new Set(rules.filter((r) => r.ruleset_source).map((r) => r.ruleset_source)), + ]; + + protectionDescription = `Branch "${branchToCheck}" is protected with: ${protectionTypes.join(', ')}. ${rulesetSources.length > 0 ? `Source: ${rulesetSources.join(', ')}` : ''}`; + protectionEvidence = { + source: 'rules_endpoint', + branch: branchToCheck, + rules, + rule_types: rules.map((r) => r.type), + ruleset_sources: rulesetSources, + }; + ctx.log(`[Strategy 1] SUCCESS - Found protection via rules endpoint`); + } else { + ctx.log(`[Strategy 1] No PR or non-fast-forward rules found`); + } + } catch (err) { + const errorMsg = err instanceof Error ? err.message : String(err); + ctx.warn(`[Strategy 1] FAILED: ${errorMsg}`); + } + + // Strategy 2: Check rulesets directly + if (!isProtected) { + ctx.log(`[Strategy 2] Trying /repos/${repo.full_name}/rulesets`); + try { + const rulesets = await ctx.fetch( + `/repos/${repo.full_name}/rulesets`, + ); + + ctx.log(`[Strategy 2] Got ${rulesets.length} rulesets`); + + const applicableRulesets = rulesets.filter( + (rs) => + rs.enforcement === 'active' && + rs.target === 'branch' && + branchMatchesRuleset(rs, branchToCheck), + ); + ctx.log( - `[Strategy 2] Ruleset "${rs.name}": rules=${JSON.stringify(rs.rules?.map((r) => r.type))}`, + `[Strategy 2] ${applicableRulesets.length} active rulesets apply to "${branchToCheck}"`, ); - } - for (const ruleset of applicableRulesets) { - const hasPullRequest = ruleset.rules?.some((r) => r.type === 'pull_request'); - if (hasPullRequest) { - isProtected = true; - const pullRequestRule = ruleset.rules?.find((r) => r.type === 'pull_request'); - protectionDescription = `Branch "${branchToCheck}" is protected by ruleset "${ruleset.name}" requiring pull request reviews.`; - protectionEvidence = { - source: 'rulesets_endpoint', - branch: branchToCheck, - ruleset_name: ruleset.name, - ruleset_id: ruleset.id, - enforcement: ruleset.enforcement, - rules: ruleset.rules, - pull_request_params: pullRequestRule?.parameters, - }; - break; + for (const rs of applicableRulesets) { + ctx.log( + `[Strategy 2] Ruleset "${rs.name}": rules=${JSON.stringify(rs.rules?.map((r) => r.type))}`, + ); + } + + for (const ruleset of applicableRulesets) { + const hasPullRequest = ruleset.rules?.some((r) => r.type === 'pull_request'); + if (hasPullRequest) { + isProtected = true; + const pullRequestRule = ruleset.rules?.find((r) => r.type === 'pull_request'); + protectionDescription = `Branch "${branchToCheck}" is protected by ruleset "${ruleset.name}" requiring pull request reviews.`; + protectionEvidence = { + source: 'rulesets_endpoint', + branch: branchToCheck, + ruleset_name: ruleset.name, + ruleset_id: ruleset.id, + enforcement: ruleset.enforcement, + rules: ruleset.rules, + pull_request_params: pullRequestRule?.parameters, + }; + break; + } } + } catch (err) { + const errorMsg = err instanceof Error ? err.message : String(err); + ctx.warn(`[Strategy 2] FAILED: ${errorMsg}`); } - } catch (err) { - const errorMsg = err instanceof Error ? err.message : String(err); - ctx.warn(`[Strategy 2] FAILED: ${errorMsg}`); } - } - // Strategy 3: Try legacy branch protection endpoint - if (!isProtected) { - ctx.log( - `[Strategy 3] Trying /repos/${repo.full_name}/branches/${branchToCheck}/protection`, - ); - try { - const protection = await ctx.fetch( - `/repos/${repo.full_name}/branches/${branchToCheck}/protection`, + // Strategy 3: Try legacy branch protection endpoint + if (!isProtected) { + ctx.log( + `[Strategy 3] Trying /repos/${repo.full_name}/branches/${branchToCheck}/protection`, ); + try { + const protection = await ctx.fetch( + `/repos/${repo.full_name}/branches/${branchToCheck}/protection`, + ); - isProtected = true; - protectionDescription = `Branch "${branchToCheck}" requires ${protection.required_pull_request_reviews?.required_approving_review_count || 0} approving review(s) (legacy protection).`; - protectionEvidence = { - source: 'legacy_branch_protection', - branch: branchToCheck, - protection_rules: protection, - }; - ctx.log(`[Strategy 3] SUCCESS - Found legacy branch protection`); - } catch (err) { - const errorMsg = err instanceof Error ? err.message : String(err); - ctx.warn(`[Strategy 3] FAILED: ${errorMsg}`); + isProtected = true; + protectionDescription = `Branch "${branchToCheck}" requires ${protection.required_pull_request_reviews?.required_approving_review_count || 0} approving review(s) (legacy protection).`; + protectionEvidence = { + source: 'legacy_branch_protection', + branch: branchToCheck, + protection_rules: protection, + }; + ctx.log(`[Strategy 3] SUCCESS - Found legacy branch protection`); + } catch (err) { + const errorMsg = err instanceof Error ? err.message : String(err); + ctx.warn(`[Strategy 3] FAILED: ${errorMsg}`); + } } - } - // Wait for PR fetch to complete - const pullRequests = await pullRequestsPromise; - - // Build evidence for this branch - const branchEvidence: Record = { - protected: isProtected, - ...protectionEvidence, - pull_requests: pullRequests, - pull_requests_window_days: recentWindowDays, - checked_at: new Date().toISOString(), - }; - - branchResults[branchToCheck] = { - protected: isProtected, - evidence: branchEvidence, - description: isProtected - ? protectionDescription - : `Branch "${branchToCheck}" has no protection rules configured.`, - }; - } // End of branch loop - - // Emit combined result for this repo - const protectedBranches = Object.entries(branchResults) - .filter(([, r]) => r.protected) - .map(([b]) => b); - const unprotectedBranches = Object.entries(branchResults) - .filter(([, r]) => !r.protected) - .map(([b]) => b); - - // Build combined evidence: { "owner/repo": { "branch1": {...}, "branch2": {...} } } - const combinedEvidence: Record> = {}; - for (const [branch, result] of Object.entries(branchResults)) { - combinedEvidence[branch] = result.evidence; - } + // Wait for PR fetch to complete + const pullRequests = await pullRequestsPromise; + + // Build evidence for this branch + const branchEvidence: Record = { + protected: isProtected, + ...protectionEvidence, + pull_requests: pullRequests, + pull_requests_window_days: recentWindowDays, + checked_at: new Date().toISOString(), + }; + + branchResults[branchToCheck] = { + protected: isProtected, + evidence: branchEvidence, + description: isProtected + ? protectionDescription + : `Branch "${branchToCheck}" has no protection rules configured.`, + }; + } // End of branch loop + + // Emit combined result for this repo + const protectedBranches = Object.entries(branchResults) + .filter(([, r]) => r.protected) + .map(([b]) => b); + const unprotectedBranches = Object.entries(branchResults) + .filter(([, r]) => !r.protected) + .map(([b]) => b); + + // Build combined evidence: { "owner/repo": { "branch1": {...}, "branch2": {...} } } + const combinedEvidence: Record> = {}; + for (const [branch, result] of Object.entries(branchResults)) { + combinedEvidence[branch] = result.evidence; + } - if (unprotectedBranches.length === 0) { - // All branches protected - ctx.pass({ - title: `All branches protected on ${repo.name}`, - description: `${protectedBranches.length} branch(es) have protection enabled: ${protectedBranches.join(', ')}`, - resourceType: 'repository', - resourceId: repo.full_name, - evidence: { - [repo.full_name]: combinedEvidence, - }, - }); - } else if (protectedBranches.length === 0) { - // No branches protected - ctx.fail({ - title: `No branch protection on ${repo.name}`, - description: `${unprotectedBranches.length} branch(es) have no protection: ${unprotectedBranches.join(', ')}`, - resourceType: 'repository', - resourceId: repo.full_name, - severity: 'high', - remediation: `1. Go to ${repo.html_url}/settings/rules\n2. Create rulesets for branches: ${unprotectedBranches.join(', ')}\n3. Enable "Require a pull request before merging"\n4. Set required approvals to at least 1`, - evidence: { - [repo.full_name]: combinedEvidence, - }, - }); - } else { - // Mixed: some protected, some not - ctx.fail({ - title: `Partial branch protection on ${repo.name}`, - description: `Protected: ${protectedBranches.join(', ')}. Unprotected: ${unprotectedBranches.join(', ')}`, - resourceType: 'repository', - resourceId: repo.full_name, - severity: 'high', - remediation: `1. Go to ${repo.html_url}/settings/rules\n2. Create rulesets for unprotected branches: ${unprotectedBranches.join(', ')}\n3. Enable "Require a pull request before merging"`, - evidence: { - [repo.full_name]: combinedEvidence, - }, - }); - } - } + if (unprotectedBranches.length === 0) { + // All branches protected + ctx.pass({ + title: `All branches protected on ${repo.name}`, + description: `${protectedBranches.length} branch(es) have protection enabled: ${protectedBranches.join(', ')}`, + resourceType: 'repository', + resourceId: repo.full_name, + evidence: { + [repo.full_name]: combinedEvidence, + }, + }); + } else if (protectedBranches.length === 0) { + // No branches protected + ctx.fail({ + title: `No branch protection on ${repo.name}`, + description: `${unprotectedBranches.length} branch(es) have no protection: ${unprotectedBranches.join(', ')}`, + resourceType: 'repository', + resourceId: repo.full_name, + severity: 'high', + remediation: `1. Go to ${repo.html_url}/settings/rules\n2. Create rulesets for branches: ${unprotectedBranches.join(', ')}\n3. Enable "Require a pull request before merging"\n4. Set required approvals to at least 1`, + evidence: { + [repo.full_name]: combinedEvidence, + }, + }); + } else { + // Mixed: some protected, some not + ctx.fail({ + title: `Partial branch protection on ${repo.name}`, + description: `Protected: ${protectedBranches.join(', ')}. Unprotected: ${unprotectedBranches.join(', ')}`, + resourceType: 'repository', + resourceId: repo.full_name, + severity: 'high', + remediation: `1. Go to ${repo.html_url}/settings/rules\n2. Create rulesets for unprotected branches: ${unprotectedBranches.join(', ')}\n3. Enable "Require a pull request before merging"`, + evidence: { + [repo.full_name]: combinedEvidence, + }, + }); + } + }, + ); }, }; diff --git a/packages/integration-platform/src/manifests/github/checks/concurrency.ts b/packages/integration-platform/src/manifests/github/checks/concurrency.ts index 2859478b5e..2c05acae7e 100644 --- a/packages/integration-platform/src/manifests/github/checks/concurrency.ts +++ b/packages/integration-platform/src/manifests/github/checks/concurrency.ts @@ -15,6 +15,18 @@ /** Per-repo file reads to keep in flight at once. */ export const FILE_READ_CONCURRENCY = 10; +/** + * Repositories to check in parallel within a single run. The branch-protection + * check fetches protection rules (up to 3 strategies) plus recent PRs per + * repo/branch; running repos SERIALLY meant an org with many monitored repos + * (~28) issued ~250 sequential GitHub calls — with retry backoff stacking on + * 429s — and blew past the synchronous manual-run HTTP timeout, so the run died + * with no result persisted. A bounded pool keeps the run well under that ceiling + * while staying far below GitHub's concurrent-request limit (each repo makes at + * most ~2 requests in flight, so this caps total concurrency at ~16). + */ +export const REPO_CHECK_CONCURRENCY = 8; + /** Run `fn` over `items` with at most `limit` in flight, preserving order. */ export async function mapWithConcurrency( items: T[],