From ea6083661077c069481a9a0fd6b8d3f1b14cb82b Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Mon, 12 Jan 2026 23:42:40 +0000 Subject: [PATCH 1/5] feat: Configure GitHub artifact provider to filter by workflow and name Co-authored-by: burak.kaya --- .craft.yml | 7 + .github/workflows/build.yml | 32 +- docs/src/content/docs/configuration.md | 75 +++ package.json | 1 + pnpm-lock.yaml | 17 + .../__tests__/github.test.ts | 426 ++++++++++++++++-- src/artifact_providers/github.ts | 332 +++++++++++++- src/schemas/project_config.ts | 32 ++ 8 files changed, 849 insertions(+), 73 deletions(-) diff --git a/.craft.yml b/.craft.yml index cc9ffee4..6b57065f 100644 --- a/.craft.yml +++ b/.craft.yml @@ -1,6 +1,13 @@ minVersion: '2.14.0' changelog: policy: auto +artifactProvider: + name: github + config: + artifacts: + Build & Test: + - craft-binary + - craft-docs preReleaseCommand: >- node -p " const {execSync} = require('child_process'); diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 69145582..c408f3af 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -80,7 +80,7 @@ jobs: - name: Upload Build Artifact uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4 with: - name: ${{ github.sha }}-build + name: craft-binary path: | ${{ github.workspace }}/*.tgz ${{ github.workspace }}/dist/craft @@ -114,33 +114,5 @@ jobs: - name: Upload Docs Artifact uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4 with: - name: ${{ github.sha }}-docs + name: craft-docs path: ${{ github.workspace }}/gh-pages.zip - - merge-artifacts: - name: Merge Artifacts - needs: - - build - - docs - if: always() && needs.build.result == 'success' && needs.docs.result == 'success' - runs-on: ubuntu-latest - permissions: - contents: read - steps: - - name: Download Build Artifacts - uses: actions/download-artifact@d3f86a106a0bac45b974a628896c90dbdf5c8093 # v4 - with: - name: ${{ github.sha }}-build - - name: Download Docs Artifacts - uses: actions/download-artifact@d3f86a106a0bac45b974a628896c90dbdf5c8093 # v4 - with: - name: ${{ github.sha }}-docs - - name: Archive Merged Artifacts - uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4 - with: - name: ${{ github.sha }} - path: | - ${{ github.workspace }}/*.tgz - ${{ github.workspace }}/dist/craft - ${{ github.workspace }}/gh-pages.zip - if-no-files-found: ignore diff --git a/docs/src/content/docs/configuration.md b/docs/src/content/docs/configuration.md index ebeeabab..30d0d249 100644 --- a/docs/src/content/docs/configuration.md +++ b/docs/src/content/docs/configuration.md @@ -390,6 +390,81 @@ artifactProvider: name: github # or 'gcs' or 'none' ``` +### GitHub Artifact Provider Configuration + +By default, the GitHub artifact provider looks for artifacts named exactly as the commit SHA. You can customize this with the `artifacts` configuration option. + +#### Pattern Syntax + +- **Regex patterns**: Wrapped in `/` (e.g., `/^build-.*$/`) +- **Exact strings**: Plain text (e.g., `build`, `release-artifacts`) + +#### Configuration Formats + +**1. Single artifact pattern** - searches all workflows: + +```yaml +artifactProvider: + name: github + config: + artifacts: /^sentry-.*\.tgz$/ +``` + +**2. Multiple artifact patterns** - searches all workflows: + +```yaml +artifactProvider: + name: github + config: + artifacts: + - /^sentry-.*\.tgz$/ + - release-bundle +``` + +**3. Workflow-scoped patterns** - filter by workflow name: + +```yaml +artifactProvider: + name: github + config: + artifacts: + build: release-artifacts # exact workflow → exact artifact + /^build-.*$/: artifacts # workflow pattern → exact artifact + ci: # exact workflow → multiple artifacts + - /^output-.*$/ + - bundle + /^release-.*$/: # workflow pattern → multiple artifacts + - /^dist-.*$/ + - checksums +``` + +#### Common Examples + +Fetch artifacts named `craft-binary` and `craft-docs` from the "Build & Test" workflow: + +```yaml +artifactProvider: + name: github + config: + artifacts: + Build & Test: + - craft-binary + - craft-docs +``` + +Fetch all `.tgz` files from any workflow: + +```yaml +artifactProvider: + name: github + config: + artifacts: /\.tgz$/ +``` + +#### Backward Compatibility + +When `artifacts` is not configured, the provider uses the legacy behavior where it searches for an artifact with a name matching the commit SHA exactly. This ensures existing configurations continue to work without changes. + ## Targets List release targets in your configuration: diff --git a/package.json b/package.json index 5fd53a34..4879293e 100644 --- a/package.json +++ b/package.json @@ -94,6 +94,7 @@ }, "dependencies": { "marked": "^17.0.1", + "p-limit": "^6.2.0", "semver": "^7.7.3" } } diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index f992f69f..6cc09276 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -11,6 +11,9 @@ importers: marked: specifier: ^17.0.1 version: 17.0.1 + p-limit: + specifier: ^6.2.0 + version: 6.2.0 semver: specifier: ^7.7.3 version: 7.7.3 @@ -2620,6 +2623,10 @@ packages: resolution: {integrity: sha512-TYOanM3wGwNGsZN2cVTYPArw454xnXj5qmWF1bEoAc4+cU/ol7GVh7odevjp1FNHduHc3KZMcFduxU5Xc6uJRQ==} engines: {node: '>=10'} + p-limit@6.2.0: + resolution: {integrity: sha512-kuUqqHNUqoIWp/c467RI4X6mmyuojY5jGutNU0wVTmEOOfcuwLqyMVoAi9MKi2Ak+5i9+nhmrK4ufZE8069kHA==} + engines: {node: '>=18'} + p-locate@5.0.0: resolution: {integrity: sha512-LaNjtRWUBY++zB5nE/NwcaoMylSPk+S+ZHNB1TzdbMJMny6dynpAGt7X/tl/QYq3TIeE6nxHppbo2LGymrG5Pw==} engines: {node: '>=10'} @@ -3209,6 +3216,10 @@ packages: resolution: {integrity: sha512-rVksvsnNCdJ/ohGc6xgPwyN8eheCxsiLM8mxuE/t/mOVqJewPuO1miLpTHQiRgTKCLexL4MeAFVagts7HmNZ2Q==} engines: {node: '>=10'} + yocto-queue@1.2.2: + resolution: {integrity: sha512-4LCcse/U2MHZ63HAJVE+v71o7yOdIe4cZ70Wpf8D/IyjDKYQLV5GD46B+hSTjJsvV5PztjvHoU580EftxjDZFQ==} + engines: {node: '>=12.20'} + zod@3.25.76: resolution: {integrity: sha512-gzUt/qt81nXsFGKIFcC3YnfEAx5NkunCfnDlvuBSSFS02bcXu4Lmea0AFIUwbLWxWPx3d9p8S5QoaujKcNQxcQ==} @@ -6059,6 +6070,10 @@ snapshots: dependencies: yocto-queue: 0.1.0 + p-limit@6.2.0: + dependencies: + yocto-queue: 1.2.2 + p-locate@5.0.0: dependencies: p-limit: 3.1.0 @@ -6645,4 +6660,6 @@ snapshots: yocto-queue@0.1.0: {} + yocto-queue@1.2.2: {} + zod@3.25.76: {} diff --git a/src/artifact_providers/__tests__/github.test.ts b/src/artifact_providers/__tests__/github.test.ts index 73103129..6311c742 100644 --- a/src/artifact_providers/__tests__/github.test.ts +++ b/src/artifact_providers/__tests__/github.test.ts @@ -1,11 +1,23 @@ -import { vi, type Mock, type MockInstance, type Mocked, type MockedFunction } from 'vitest'; +import { + vi, + type Mock, + type MockedFunction, + describe, + test, + expect, + beforeEach, +} from 'vitest'; vi.mock('../../utils/githubApi.ts'); import { getGitHubClient } from '../../utils/githubApi'; import { GitHubArtifactProvider, ArtifactItem, + WorkflowRun, lazyRequest, lazyRequestCallback, + normalizeArtifactsConfig, + patternToRegexp, + NormalizedArtifactFilter, } from '../github'; import { sleep } from '../../utils/async'; @@ -19,6 +31,21 @@ class TestGitHubArtifactProvider extends GitHubArtifactProvider { ): Promise { return this.searchForRevisionArtifact(revision, getRevisionDate); } + public testGetWorkflowRunsForCommit(revision: string): Promise { + return this.getWorkflowRunsForCommit(revision); + } + public testFilterWorkflowRuns( + runs: WorkflowRun[], + filters: NormalizedArtifactFilter[] + ): WorkflowRun[] { + return this.filterWorkflowRuns(runs, filters); + } + public testGetArtifactsFromWorkflowRuns( + runs: WorkflowRun[], + filters: NormalizedArtifactFilter[] + ): Promise { + return this.getArtifactsFromWorkflowRuns(runs, filters); + } } vi.mock('../../utils/async'); @@ -28,24 +55,30 @@ describe('GitHub Artifact Provider', () => { let mockClient: { actions: { listArtifactsForRepo: Mock; + listWorkflowRunsForRepo: Mock; + listWorkflowRunArtifacts: Mock; }; git: { getCommit: Mock; }; }; - let mockedSleep; + let mockedSleep: Mock; beforeEach(() => { vi.resetAllMocks(); mockClient = { - actions: { listArtifactsForRepo: vi.fn() }, + actions: { + listArtifactsForRepo: vi.fn(), + listWorkflowRunsForRepo: vi.fn(), + listWorkflowRunArtifacts: vi.fn(), + }, git: { getCommit: vi.fn() }, }; - (getGitHubClient as MockedFunction< - typeof getGitHubClient + ( + getGitHubClient as MockedFunction // @ts-ignore we only need to mock a subset - >).mockReturnValueOnce(mockClient); + ).mockReturnValueOnce(mockClient); githubArtifactProvider = new TestGitHubArtifactProvider({ name: 'github-test', @@ -59,6 +92,103 @@ describe('GitHub Artifact Provider', () => { }); }); + describe('patternToRegexp', () => { + test('converts regex string to RegExp', () => { + const result = patternToRegexp('/^build-.*$/'); + expect(result).toBeInstanceOf(RegExp); + expect(result.test('build-linux')).toBe(true); + expect(result.test('build-macos')).toBe(true); + expect(result.test('test-linux')).toBe(false); + }); + + test('converts regex string with modifiers', () => { + const result = patternToRegexp('/BUILD/i'); + expect(result.test('build')).toBe(true); + expect(result.test('BUILD')).toBe(true); + }); + + test('converts exact string to exact match RegExp', () => { + const result = patternToRegexp('build'); + expect(result.test('build')).toBe(true); + expect(result.test('build-linux')).toBe(false); + expect(result.test('test')).toBe(false); + }); + + test('escapes special characters in exact match', () => { + const result = patternToRegexp('output.tar.gz'); + expect(result.test('output.tar.gz')).toBe(true); + expect(result.test('outputXtarXgz')).toBe(false); + }); + }); + + describe('normalizeArtifactsConfig', () => { + test('returns empty array for undefined config', () => { + expect(normalizeArtifactsConfig(undefined)).toEqual([]); + }); + + test('normalizes string config to array with single filter', () => { + const result = normalizeArtifactsConfig('/^sentry-.*\\.tgz$/'); + expect(result).toHaveLength(1); + expect(result[0].workflow).toBeUndefined(); + expect(result[0].artifacts).toHaveLength(1); + expect(result[0].artifacts[0].test('sentry-browser-7.0.0.tgz')).toBe(true); + }); + + test('normalizes array config to single filter with multiple patterns', () => { + const result = normalizeArtifactsConfig([ + '/^sentry-.*\\.tgz$/', + 'release-bundle', + ]); + expect(result).toHaveLength(1); + expect(result[0].workflow).toBeUndefined(); + expect(result[0].artifacts).toHaveLength(2); + expect(result[0].artifacts[0].test('sentry-browser-7.0.0.tgz')).toBe(true); + expect(result[0].artifacts[1].test('release-bundle')).toBe(true); + expect(result[0].artifacts[1].test('release-bundle-extra')).toBe(false); + }); + + test('normalizes object config with exact workflow names', () => { + const result = normalizeArtifactsConfig({ + build: 'release-artifacts', + ci: ['output', 'bundle'], + }); + expect(result).toHaveLength(2); + + // First filter: build -> release-artifacts + expect(result[0].workflow?.test('build')).toBe(true); + expect(result[0].workflow?.test('build-linux')).toBe(false); + expect(result[0].artifacts).toHaveLength(1); + expect(result[0].artifacts[0].test('release-artifacts')).toBe(true); + + // Second filter: ci -> [output, bundle] + expect(result[1].workflow?.test('ci')).toBe(true); + expect(result[1].artifacts).toHaveLength(2); + expect(result[1].artifacts[0].test('output')).toBe(true); + expect(result[1].artifacts[1].test('bundle')).toBe(true); + }); + + test('normalizes object config with workflow patterns', () => { + const result = normalizeArtifactsConfig({ + '/^build-.*$/': '/^output-.*$/', + '/^release-.*$/': ['/^dist-.*$/', 'checksums'], + }); + expect(result).toHaveLength(2); + + // First filter: /^build-.*$/ -> /^output-.*$/ + expect(result[0].workflow?.test('build-linux')).toBe(true); + expect(result[0].workflow?.test('build-macos')).toBe(true); + expect(result[0].workflow?.test('test-linux')).toBe(false); + expect(result[0].artifacts[0].test('output-x86')).toBe(true); + expect(result[0].artifacts[0].test('output-arm')).toBe(true); + + // Second filter: /^release-.*$/ -> [/^dist-.*$/, checksums] + expect(result[1].workflow?.test('release-production')).toBe(true); + expect(result[1].artifacts).toHaveLength(2); + expect(result[1].artifacts[0].test('dist-linux')).toBe(true); + expect(result[1].artifacts[1].test('checksums')).toBe(true); + }); + }); + describe('getRevisionArtifact', () => { test('it should get the artifact with the revision name from the first page', async () => { mockClient.actions.listArtifactsForRepo.mockResolvedValueOnce({ @@ -71,8 +201,7 @@ describe('GitHub Artifact Provider', () => { node_id: 'MDg6QXJ0aWZhY3Q2MDIzMzcxMA==', name: '1b843f2cbb20fdda99ef749e29e75e43e6e43b38', size_in_bytes: 6511029, - url: - 'https://api.github.com/repos/getsentry/craft/actions/artifacts/60233710', + url: 'https://api.github.com/repos/getsentry/craft/actions/artifacts/60233710', archive_download_url: 'https://api.github.com/repos/getsentry/craft/actions/artifacts/60233710/zip', expired: false, @@ -85,8 +214,7 @@ describe('GitHub Artifact Provider', () => { node_id: 'MDg6QXJ0aWZhY3Q2MDIzMjY5MQ==', name: 'e4bcfe450e0460ec5f20b20868664171effef6f9', size_in_bytes: 6511029, - url: - 'https://api.github.com/repos/getsentry/craft/actions/artifacts/60232691', + url: 'https://api.github.com/repos/getsentry/craft/actions/artifacts/60232691', archive_download_url: 'https://api.github.com/repos/getsentry/craft/actions/artifacts/60232691/zip', expired: false, @@ -98,7 +226,6 @@ describe('GitHub Artifact Provider', () => { }, }); await expect( - // TODO(sentry): Could not automatically migrate - see https://github.com/getsentry/sentry-javascript/blob/develop/MIGRATION.md#deprecate-hub githubArtifactProvider.testGetRevisionArtifact( '1b843f2cbb20fdda99ef749e29e75e43e6e43b38' ) @@ -139,8 +266,7 @@ describe('GitHub Artifact Provider', () => { node_id: 'MDg6QXJ0aWZhY3Q2MDIzMjY5MQ==', name: 'e4bcfe450e0460ec5f20b20868664171effef6f9', size_in_bytes: 6511029, - url: - 'https://api.github.com/repos/getsentry/craft/actions/artifacts/60232691', + url: 'https://api.github.com/repos/getsentry/craft/actions/artifacts/60232691', archive_download_url: 'https://api.github.com/repos/getsentry/craft/actions/artifacts/60232691/zip', expired: false, @@ -161,8 +287,7 @@ describe('GitHub Artifact Provider', () => { node_id: 'MDg6QXJ0aWZhY3Q2MDIzMzcxMA==', name: '1b843f2cbb20fdda99ef749e29e75e43e6e43b38', size_in_bytes: 6511029, - url: - 'https://api.github.com/repos/getsentry/craft/actions/artifacts/60233710', + url: 'https://api.github.com/repos/getsentry/craft/actions/artifacts/60233710', archive_download_url: 'https://api.github.com/repos/getsentry/craft/actions/artifacts/60233710/zip', expired: false, @@ -174,7 +299,6 @@ describe('GitHub Artifact Provider', () => { }, }); await expect( - // TODO(sentry): Could not automatically migrate - see https://github.com/getsentry/sentry-javascript/blob/develop/MIGRATION.md#deprecate-hub githubArtifactProvider.testGetRevisionArtifact( '1b843f2cbb20fdda99ef749e29e75e43e6e43b38' ) @@ -206,8 +330,7 @@ describe('GitHub Artifact Provider', () => { node_id: 'MDg6QXJ0aWZhY3Q2MDIzMzcxMA==', name: '1b843f2cbb20fdda99ef749e29e75e43e6e43b38', size_in_bytes: 6511029, - url: - 'https://api.github.com/repos/getsentry/craft/actions/artifacts/60233710', + url: 'https://api.github.com/repos/getsentry/craft/actions/artifacts/60233710', archive_download_url: 'https://api.github.com/repos/getsentry/craft/actions/artifacts/60233710/zip', expired: false, @@ -220,8 +343,7 @@ describe('GitHub Artifact Provider', () => { node_id: 'MDg6QXJ0aWZhY3Q2MDIzMjY5MQ==', name: '1b843f2cbb20fdda99ef749e29e75e43e6e43b38', size_in_bytes: 6511029, - url: - 'https://api.github.com/repos/getsentry/craft/actions/artifacts/60232691', + url: 'https://api.github.com/repos/getsentry/craft/actions/artifacts/60232691', archive_download_url: 'https://api.github.com/repos/getsentry/craft/actions/artifacts/60232691/zip', expired: false, @@ -233,7 +355,6 @@ describe('GitHub Artifact Provider', () => { }, }); await expect( - // TODO(sentry): Could not automatically migrate - see https://github.com/getsentry/sentry-javascript/blob/develop/MIGRATION.md#deprecate-hub githubArtifactProvider.testGetRevisionArtifact( '1b843f2cbb20fdda99ef749e29e75e43e6e43b38' ) @@ -263,7 +384,6 @@ describe('GitHub Artifact Provider', () => { }); await expect( - // TODO(sentry): Could not automatically migrate - see https://github.com/getsentry/sentry-javascript/blob/develop/MIGRATION.md#deprecate-hub githubArtifactProvider.testGetRevisionArtifact( '1b843f2cbb20fdda99ef749e29e75e43e6e43b38' ) @@ -286,8 +406,7 @@ describe('GitHub Artifact Provider', () => { node_id: 'MDg6QXJ0aWZhY3Q2MDIzMzcxMA==', name: '1b843f2cbb20fdda99ef749e29e75e43e6e43b38', size_in_bytes: 6511029, - url: - 'https://api.github.com/repos/getsentry/craft/actions/artifacts/60233710', + url: 'https://api.github.com/repos/getsentry/craft/actions/artifacts/60233710', archive_download_url: 'https://api.github.com/repos/getsentry/craft/actions/artifacts/60233710/zip', expired: false, @@ -300,8 +419,7 @@ describe('GitHub Artifact Provider', () => { node_id: 'MDg6QXJ0aWZhY3Q2MDIzMjY5MQ==', name: '1b843f2cbb20fdda99ef749e29e75e43e6e43b38', size_in_bytes: 6511029, - url: - 'https://api.github.com/repos/getsentry/craft/actions/artifacts/60232691', + url: 'https://api.github.com/repos/getsentry/craft/actions/artifacts/60232691', archive_download_url: 'https://api.github.com/repos/getsentry/craft/actions/artifacts/60232691/zip', expired: false, @@ -313,7 +431,6 @@ describe('GitHub Artifact Provider', () => { }, }); await expect( - // TODO(sentry): Could not automatically migrate - see https://github.com/getsentry/sentry-javascript/blob/develop/MIGRATION.md#deprecate-hub githubArtifactProvider.testGetRevisionArtifact( '3c2e87573d3bd16f61cf08fece0638cc47a4fc22' ) @@ -338,8 +455,7 @@ describe('GitHub Artifact Provider', () => { node_id: 'MDg6QXJ0aWZhY3Q2MDIzMjY5MQ==', name: 'e4bcfe450e0460ec5f20b20868664171effef6f9', size_in_bytes: 6511029, - url: - 'https://api.github.com/repos/getsentry/craft/actions/artifacts/60232691', + url: 'https://api.github.com/repos/getsentry/craft/actions/artifacts/60232691', archive_download_url: 'https://api.github.com/repos/getsentry/craft/actions/artifacts/60232691/zip', expired: false, @@ -360,8 +476,7 @@ describe('GitHub Artifact Provider', () => { node_id: 'MDg6QXJ0aWZhY3Q2MDIzMzcxMA==', name: '1b843f2cbb20fdda99ef749e29e75e43e6e43b38', size_in_bytes: 6511029, - url: - 'https://api.github.com/repos/getsentry/craft/actions/artifacts/60233710', + url: 'https://api.github.com/repos/getsentry/craft/actions/artifacts/60233710', archive_download_url: 'https://api.github.com/repos/getsentry/craft/actions/artifacts/60233710/zip', expired: false, @@ -378,7 +493,6 @@ describe('GitHub Artifact Provider', () => { .mockResolvedValueOnce('2020-05-12T21:45:04Z'); await expect( - // TODO(sentry): Could not automatically migrate - see https://github.com/getsentry/sentry-javascript/blob/develop/MIGRATION.md#deprecate-hub githubArtifactProvider.testSearchForRevisionArtifact( '1b843f2cbb20fdda99ef749e29e75e43e6e43b38', lazyRequest(() => { @@ -416,8 +530,7 @@ describe('GitHub Artifact Provider', () => { node_id: 'MDg6QXJ0aWZhY3Q2MDIzMjY5MQ==', name: 'e4bcfe450e0460ec5f20b20868664171effef6f9', size_in_bytes: 6511029, - url: - 'https://api.github.com/repos/getsentry/craft/actions/artifacts/60232691', + url: 'https://api.github.com/repos/getsentry/craft/actions/artifacts/60232691', archive_download_url: 'https://api.github.com/repos/getsentry/craft/actions/artifacts/60232691/zip', expired: false, @@ -438,8 +551,7 @@ describe('GitHub Artifact Provider', () => { node_id: 'MDg6QXJ0aWZhY3Q2MDIzMjY5MQ==', name: 'e4bcfe450e0460ec5f20b20868664171effef6f9', size_in_bytes: 6511029, - url: - 'https://api.github.com/repos/getsentry/craft/actions/artifacts/60232691', + url: 'https://api.github.com/repos/getsentry/craft/actions/artifacts/60232691', archive_download_url: 'https://api.github.com/repos/getsentry/craft/actions/artifacts/60232691/zip', expired: false, @@ -460,8 +572,7 @@ describe('GitHub Artifact Provider', () => { node_id: 'MDg6QXJ0aWZhY3Q2MDIzMjY5MQ==', name: 'e4bcfe450e0460ec5f20b20868664171effef6f9', size_in_bytes: 6511029, - url: - 'https://api.github.com/repos/getsentry/craft/actions/artifacts/60232691', + url: 'https://api.github.com/repos/getsentry/craft/actions/artifacts/60232691', archive_download_url: 'https://api.github.com/repos/getsentry/craft/actions/artifacts/60232691/zip', expired: false, @@ -478,7 +589,6 @@ describe('GitHub Artifact Provider', () => { .mockResolvedValueOnce('2020-05-12T21:45:04Z'); await expect( - // TODO(sentry): Could not automatically migrate - see https://github.com/getsentry/sentry-javascript/blob/develop/MIGRATION.md#deprecate-hub githubArtifactProvider.testSearchForRevisionArtifact( '1b843f2cbb20fdda99ef749e29e75e43e6e43b38', lazyRequest(getRevisionDateCallback) @@ -500,8 +610,7 @@ describe('GitHub Artifact Provider', () => { node_id: 'MDg6QXJ0aWZhY3Q2MDIzMjY5MQ==', name: 'e4bcfe450e0460ec5f20b20868664171effef6f9', size_in_bytes: 6511029, - url: - 'https://api.github.com/repos/getsentry/craft/actions/artifacts/60232691', + url: 'https://api.github.com/repos/getsentry/craft/actions/artifacts/60232691', archive_download_url: 'https://api.github.com/repos/getsentry/craft/actions/artifacts/60232691/zip', expired: false, @@ -518,7 +627,6 @@ describe('GitHub Artifact Provider', () => { .mockResolvedValueOnce('2021-05-12T21:45:04Z'); await expect( - // TODO(sentry): Could not automatically migrate - see https://github.com/getsentry/sentry-javascript/blob/develop/MIGRATION.md#deprecate-hub githubArtifactProvider.testSearchForRevisionArtifact( '1b843f2cbb20fdda99ef749e29e75e43e6e43b38', lazyRequest(() => { @@ -530,4 +638,238 @@ describe('GitHub Artifact Provider', () => { expect(getRevisionDateCallback).toBeCalledTimes(1); }); }); + + describe('getWorkflowRunsForCommit', () => { + test('fetches workflow runs for a commit', async () => { + mockClient.actions.listWorkflowRunsForRepo.mockResolvedValueOnce({ + status: 200, + data: { + total_count: 2, + workflow_runs: [ + { id: 1, name: 'Build & Test' }, + { id: 2, name: 'Lint' }, + ], + }, + }); + + const runs = await githubArtifactProvider.testGetWorkflowRunsForCommit( + 'abc123' + ); + + expect(runs).toHaveLength(2); + expect(runs[0].name).toBe('Build & Test'); + expect(runs[1].name).toBe('Lint'); + expect(mockClient.actions.listWorkflowRunsForRepo).toBeCalledWith({ + owner: 'getsentry', + repo: 'craft', + head_sha: 'abc123', + per_page: 100, + page: 1, + }); + }); + + test('handles pagination for workflow runs', async () => { + // Create array of 100 runs for first page + const firstPageRuns = Array.from({ length: 100 }, (_, i) => ({ + id: i + 1, + name: `Workflow ${i + 1}`, + })); + + mockClient.actions.listWorkflowRunsForRepo + .mockResolvedValueOnce({ + status: 200, + data: { + total_count: 105, + workflow_runs: firstPageRuns, + }, + }) + .mockResolvedValueOnce({ + status: 200, + data: { + total_count: 105, + workflow_runs: [ + { id: 101, name: 'Workflow 101' }, + { id: 102, name: 'Workflow 102' }, + { id: 103, name: 'Workflow 103' }, + { id: 104, name: 'Workflow 104' }, + { id: 105, name: 'Workflow 105' }, + ], + }, + }); + + const runs = await githubArtifactProvider.testGetWorkflowRunsForCommit( + 'abc123' + ); + + expect(runs).toHaveLength(105); + expect(mockClient.actions.listWorkflowRunsForRepo).toBeCalledTimes(2); + }); + }); + + describe('filterWorkflowRuns', () => { + const mockRuns = [ + { id: 1, name: 'Build & Test' }, + { id: 2, name: 'build-linux' }, + { id: 3, name: 'build-macos' }, + { id: 4, name: 'Lint' }, + ] as WorkflowRun[]; + + test('returns all runs when no workflow filters are specified', () => { + const filters: NormalizedArtifactFilter[] = [ + { workflow: undefined, artifacts: [/^output$/] }, + ]; + + const result = githubArtifactProvider.testFilterWorkflowRuns( + mockRuns, + filters + ); + expect(result).toHaveLength(4); + }); + + test('filters runs by exact workflow name', () => { + const filters: NormalizedArtifactFilter[] = [ + { workflow: /^Build & Test$/, artifacts: [/^output$/] }, + ]; + + const result = githubArtifactProvider.testFilterWorkflowRuns( + mockRuns, + filters + ); + expect(result).toHaveLength(1); + expect(result[0].name).toBe('Build & Test'); + }); + + test('filters runs by workflow pattern', () => { + const filters: NormalizedArtifactFilter[] = [ + { workflow: /^build-.*$/, artifacts: [/^output$/] }, + ]; + + const result = githubArtifactProvider.testFilterWorkflowRuns( + mockRuns, + filters + ); + expect(result).toHaveLength(2); + expect(result.map(r => r.name)).toEqual(['build-linux', 'build-macos']); + }); + + test('combines multiple workflow filters', () => { + const filters: NormalizedArtifactFilter[] = [ + { workflow: /^Build & Test$/, artifacts: [/^output$/] }, + { workflow: /^Lint$/, artifacts: [/^report$/] }, + ]; + + const result = githubArtifactProvider.testFilterWorkflowRuns( + mockRuns, + filters + ); + expect(result).toHaveLength(2); + expect(result.map(r => r.name)).toEqual(['Build & Test', 'Lint']); + }); + }); + + describe('getArtifactsFromWorkflowRuns', () => { + test('fetches and filters artifacts from workflow runs', async () => { + const mockRuns = [ + { id: 1, name: 'Build & Test' }, + { id: 2, name: 'Lint' }, + ] as WorkflowRun[]; + + mockClient.actions.listWorkflowRunArtifacts + .mockResolvedValueOnce({ + status: 200, + data: { + total_count: 2, + artifacts: [ + { id: 101, name: 'craft-binary' }, + { id: 102, name: 'craft-docs' }, + ], + }, + }) + .mockResolvedValueOnce({ + status: 200, + data: { + total_count: 1, + artifacts: [{ id: 201, name: 'lint-report' }], + }, + }); + + const filters: NormalizedArtifactFilter[] = [ + { workflow: /^Build & Test$/, artifacts: [/^craft-/] }, + ]; + + const artifacts = + await githubArtifactProvider.testGetArtifactsFromWorkflowRuns( + mockRuns, + filters + ); + + expect(artifacts).toHaveLength(2); + expect(artifacts.map(a => a.name)).toEqual(['craft-binary', 'craft-docs']); + }); + + test('matches artifacts without workflow filter (all workflows)', async () => { + const mockRuns = [ + { id: 1, name: 'Build & Test' }, + { id: 2, name: 'Lint' }, + ] as WorkflowRun[]; + + mockClient.actions.listWorkflowRunArtifacts + .mockResolvedValueOnce({ + status: 200, + data: { + total_count: 1, + artifacts: [{ id: 101, name: 'craft-binary' }], + }, + }) + .mockResolvedValueOnce({ + status: 200, + data: { + total_count: 1, + artifacts: [{ id: 201, name: 'craft-report' }], + }, + }); + + const filters: NormalizedArtifactFilter[] = [ + { workflow: undefined, artifacts: [/^craft-/] }, + ]; + + const artifacts = + await githubArtifactProvider.testGetArtifactsFromWorkflowRuns( + mockRuns, + filters + ); + + expect(artifacts).toHaveLength(2); + expect(artifacts.map(a => a.name)).toEqual([ + 'craft-binary', + 'craft-report', + ]); + }); + + test('does not add duplicate artifacts', async () => { + const mockRuns = [{ id: 1, name: 'Build & Test' }] as WorkflowRun[]; + + mockClient.actions.listWorkflowRunArtifacts.mockResolvedValueOnce({ + status: 200, + data: { + total_count: 1, + artifacts: [{ id: 101, name: 'craft-binary' }], + }, + }); + + // Two filters that both match the same artifact + const filters: NormalizedArtifactFilter[] = [ + { workflow: /^Build/, artifacts: [/^craft-/] }, + { workflow: undefined, artifacts: [/binary$/] }, + ]; + + const artifacts = + await githubArtifactProvider.testGetArtifactsFromWorkflowRuns( + mockRuns, + filters + ); + + expect(artifacts).toHaveLength(1); + }); + }); }); diff --git a/src/artifact_providers/github.ts b/src/artifact_providers/github.ts index 3dea85ae..29a118c6 100644 --- a/src/artifact_providers/github.ts +++ b/src/artifact_providers/github.ts @@ -2,6 +2,7 @@ import { Octokit, RestEndpointMethodTypes } from '@octokit/rest'; import * as fs from 'fs'; import fetch from 'node-fetch'; import * as path from 'path'; +import pLimit from 'p-limit'; import { ArtifactProviderConfig, @@ -17,12 +18,98 @@ import { } from '../utils/files'; import { extractZipArchive } from '../utils/system'; import { sleep } from '../utils/async'; +import { stringToRegexp } from '../utils/filters'; +import { GitHubArtifactsConfig } from '../schemas/project_config'; const MAX_TRIES = 3; const MILLISECONDS = 1000; const ARTIFACTS_POLLING_INTERVAL = 10 * MILLISECONDS; +const DOWNLOAD_CONCURRENCY = 3; -export type ArtifactItem = RestEndpointMethodTypes['actions']['listArtifactsForRepo']['response']['data']['artifacts'][0]; +export type ArtifactItem = + RestEndpointMethodTypes['actions']['listArtifactsForRepo']['response']['data']['artifacts'][0]; + +export type WorkflowRun = + RestEndpointMethodTypes['actions']['listWorkflowRunsForRepo']['response']['data']['workflow_runs'][0]; + +/** + * Normalized artifact filter structure + */ +export interface NormalizedArtifactFilter { + /** Optional workflow name pattern to match (if undefined, matches all workflows) */ + workflow?: RegExp; + /** Artifact name patterns to match */ + artifacts: RegExp[]; +} + +/** + * Converts a string to a RegExp if it's wrapped in slashes, otherwise creates an exact match RegExp + */ +export function patternToRegexp(pattern: string): RegExp { + if (pattern.startsWith('/') && pattern.lastIndexOf('/') > 0) { + return stringToRegexp(pattern); + } + // Exact match (escape special regex characters) + const escaped = pattern.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); + return new RegExp(`^${escaped}$`); +} + +/** + * Normalizes artifact patterns from string/array format to array of RegExp + */ +function normalizeArtifactPatterns( + patterns: string | string[] | undefined +): RegExp[] { + if (!patterns) { + return []; + } + const patternArray = Array.isArray(patterns) ? patterns : [patterns]; + return patternArray.map(patternToRegexp); +} + +/** + * Normalizes the artifacts config to a standard structure + * + * @param config The raw artifacts config from .craft.yml + * @returns Array of normalized filter objects + */ +export function normalizeArtifactsConfig( + config: GitHubArtifactsConfig +): NormalizedArtifactFilter[] { + if (!config) { + return []; + } + + // String format: single artifact pattern (searches all workflows) + if (typeof config === 'string') { + return [ + { + workflow: undefined, + artifacts: normalizeArtifactPatterns(config), + }, + ]; + } + + // Array format: multiple artifact patterns (searches all workflows) + if (Array.isArray(config)) { + return [ + { + workflow: undefined, + artifacts: normalizeArtifactPatterns(config), + }, + ]; + } + + // Object format: workflow-scoped patterns + const filters: NormalizedArtifactFilter[] = []; + for (const [workflowPattern, artifactPatterns] of Object.entries(config)) { + filters.push({ + workflow: patternToRegexp(workflowPattern), + artifacts: normalizeArtifactPatterns(artifactPatterns), + }); + } + return filters; +} /** * GitHub artifact provider @@ -230,12 +317,255 @@ export class GitHubArtifactProvider extends BaseArtifactProvider { return archiveResponse.url; } + /** + * Gets workflow runs for a specific commit SHA + * + * @param revision Git commit SHA + * @returns Array of workflow runs for the commit + */ + protected async getWorkflowRunsForCommit( + revision: string + ): Promise { + const { repoName: repo, repoOwner: owner } = this.config; + + this.logger.debug( + `Fetching workflow runs for commit ${revision} from ${owner}/${repo}` + ); + + const runs: WorkflowRun[] = []; + const per_page = 100; + + for (let page = 1; ; page++) { + const response = await this.github.actions.listWorkflowRunsForRepo({ + owner, + repo, + head_sha: revision, + per_page, + page, + }); + + runs.push(...response.data.workflow_runs); + + if (response.data.workflow_runs.length < per_page) { + break; + } + } + + this.logger.debug(`Found ${runs.length} workflow runs for commit`); + return runs; + } + + /** + * Filters workflow runs by name patterns + * + * @param runs Array of workflow runs + * @param filters Array of normalized filters + * @returns Filtered workflow runs that match at least one filter's workflow pattern + */ + protected filterWorkflowRuns( + runs: WorkflowRun[], + filters: NormalizedArtifactFilter[] + ): WorkflowRun[] { + // If no filters have workflow patterns, return all runs + const hasWorkflowFilters = filters.some(f => f.workflow !== undefined); + if (!hasWorkflowFilters) { + return runs; + } + + return runs.filter(run => { + const workflowName = run.name ?? ''; + return filters.some( + filter => !filter.workflow || filter.workflow.test(workflowName) + ); + }); + } + + /** + * Gets artifacts from workflow runs and filters them by patterns + * + * @param runs Array of workflow runs + * @param filters Array of normalized filters + * @returns Array of matching artifacts + */ + protected async getArtifactsFromWorkflowRuns( + runs: WorkflowRun[], + filters: NormalizedArtifactFilter[] + ): Promise { + const { repoName: repo, repoOwner: owner } = this.config; + const matchingArtifacts: ArtifactItem[] = []; + + for (const run of runs) { + const workflowName = run.name ?? ''; + this.logger.debug( + `Fetching artifacts for workflow run "${workflowName}" (ID: ${run.id})` + ); + + const per_page = 100; + for (let page = 1; ; page++) { + const response = await this.github.actions.listWorkflowRunArtifacts({ + owner, + repo, + run_id: run.id, + per_page, + page, + }); + + for (const artifact of response.data.artifacts) { + // Check if this artifact matches any filter + for (const filter of filters) { + // If filter has a workflow pattern, check if this run matches it + if (filter.workflow && !filter.workflow.test(workflowName)) { + continue; + } + + // Check if artifact name matches any of the artifact patterns + const matches = filter.artifacts.some(pattern => + pattern.test(artifact.name) + ); + if (matches) { + this.logger.debug( + `Artifact "${artifact.name}" matches filter from workflow "${workflowName}"` + ); + matchingArtifacts.push(artifact); + break; // Don't add the same artifact twice + } + } + } + + if (response.data.artifacts.length < per_page) { + break; + } + } + } + + this.logger.debug(`Found ${matchingArtifacts.length} matching artifacts`); + return matchingArtifacts; + } + + /** + * Downloads multiple artifacts in parallel and unpacks them + * + * @param artifactItems Array of artifact items to download + * @returns Array of remote artifacts from all downloaded zip files + */ + protected async downloadArtifactsInParallel( + artifactItems: ArtifactItem[] + ): Promise { + const limit = pLimit(DOWNLOAD_CONCURRENCY); + const allArtifacts: RemoteArtifact[] = []; + + const downloadPromises = artifactItems.map(item => + limit(async () => { + this.logger.debug(`Downloading artifact "${item.name}"...`); + const url = await this.getArchiveDownloadUrl(item); + const artifacts = await this.downloadAndUnpackArtifacts(url); + return artifacts; + }) + ); + + const results = await Promise.all(downloadPromises); + for (const artifacts of results) { + allArtifacts.push(...artifacts); + } + + return allArtifacts; + } + + /** + * Fetches artifacts using the new workflow-based approach + * + * @param revision Git commit SHA + * @param filters Normalized artifact filters + * @returns Array of remote artifacts + */ + protected async doListArtifactsWithFilters( + revision: string, + filters: NormalizedArtifactFilter[] + ): Promise { + for (let tries = 0; tries < MAX_TRIES; tries++) { + this.logger.info( + `Fetching GitHub artifacts for revision ${revision} using artifact filters (attempt ${ + tries + 1 + } of ${MAX_TRIES})` + ); + + // Get workflow runs for the commit + const allRuns = await this.getWorkflowRunsForCommit(revision); + if (allRuns.length === 0) { + this.logger.debug(`No workflow runs found for commit ${revision}`); + if (tries + 1 < MAX_TRIES) { + this.logger.info( + `Waiting ${ + ARTIFACTS_POLLING_INTERVAL / MILLISECONDS + } seconds for workflow runs to become available...` + ); + await sleep(ARTIFACTS_POLLING_INTERVAL); + continue; + } + throw new Error( + `No workflow runs found for revision "${revision}" (tries: ${MAX_TRIES})` + ); + } + + // Filter workflow runs by patterns + const filteredRuns = this.filterWorkflowRuns(allRuns, filters); + this.logger.debug( + `${filteredRuns.length} of ${allRuns.length} workflow runs match filters` + ); + + // Get artifacts from filtered workflow runs + const matchingArtifacts = await this.getArtifactsFromWorkflowRuns( + filteredRuns, + filters + ); + + if (matchingArtifacts.length === 0) { + this.logger.debug(`No matching artifacts found`); + if (tries + 1 < MAX_TRIES) { + this.logger.info( + `Waiting ${ + ARTIFACTS_POLLING_INTERVAL / MILLISECONDS + } seconds for artifacts to become available...` + ); + await sleep(ARTIFACTS_POLLING_INTERVAL); + continue; + } + throw new Error( + `No artifacts matching filters found for revision "${revision}" (tries: ${MAX_TRIES})` + ); + } + + // Download artifacts in parallel + this.logger.debug( + `Downloading ${matchingArtifacts.length} artifacts in parallel...` + ); + return await this.downloadArtifactsInParallel(matchingArtifacts); + } + + // This should not be reached, but TypeScript needs a return + throw new Error( + `Failed to fetch artifacts for revision "${revision}" (tries: ${MAX_TRIES})` + ); + } + /** * @inheritDoc */ protected async doListArtifactsForRevision( revision: string ): Promise { + // Check if artifacts config is present + const artifactsConfig = this.config.artifacts as + | GitHubArtifactsConfig + | undefined; + const filters = normalizeArtifactsConfig(artifactsConfig); + + if (filters.length > 0) { + // Use new workflow-based approach + return await this.doListArtifactsWithFilters(revision, filters); + } + + // Legacy behavior: artifact.name === revision SHA const foundArtifact = await this.getRevisionArtifact(revision); this.logger.debug(`Requesting archive URL from GitHub...`); diff --git a/src/schemas/project_config.ts b/src/schemas/project_config.ts index eae22834..7876a8f3 100644 --- a/src/schemas/project_config.ts +++ b/src/schemas/project_config.ts @@ -82,6 +82,38 @@ export const BaseArtifactProviderSchema = z.object({ export type BaseArtifactProvider = z.infer; +/** + * Artifact pattern(s) for a single workflow - can be a single string or array of strings + */ +const ArtifactPatternsSchema = z.union([z.string(), z.array(z.string())]); + +/** + * Full artifacts config for GitHub artifact provider: + * - string: single artifact pattern (searches all workflows) + * - array: multiple artifact patterns (searches all workflows) + * - object: workflow-scoped patterns (keys = workflow name/pattern, values = artifact(s)) + */ +export const GitHubArtifactsConfigSchema = z + .union([ + z.string(), // single artifact pattern + z.array(z.string()), // array of artifact patterns + z.record(z.string(), ArtifactPatternsSchema), // workflow → artifact(s) + ]) + .optional(); + +export type GitHubArtifactsConfig = z.infer; + +/** + * GitHub-specific artifact provider configuration + */ +export const GitHubArtifactProviderConfigSchema = z.object({ + artifacts: GitHubArtifactsConfigSchema, +}); + +export type GitHubArtifactProviderConfig = z.infer< + typeof GitHubArtifactProviderConfigSchema +>; + /** * Calendar versioning configuration */ From 85d9a310a59e6024fccdf1d583dfd1582b0200c9 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Tue, 13 Jan 2026 00:16:39 +0000 Subject: [PATCH 2/5] refactor: Remove AI-generated comment slop Remove unnecessary inline comments that were overly explanatory for self-documenting code. --- src/artifact_providers/github.ts | 31 ++++--------------------------- src/schemas/project_config.ts | 12 +++++------- 2 files changed, 9 insertions(+), 34 deletions(-) diff --git a/src/artifact_providers/github.ts b/src/artifact_providers/github.ts index 29a118c6..05244e00 100644 --- a/src/artifact_providers/github.ts +++ b/src/artifact_providers/github.ts @@ -80,27 +80,14 @@ export function normalizeArtifactsConfig( return []; } - // String format: single artifact pattern (searches all workflows) if (typeof config === 'string') { - return [ - { - workflow: undefined, - artifacts: normalizeArtifactPatterns(config), - }, - ]; + return [{ workflow: undefined, artifacts: normalizeArtifactPatterns(config) }]; } - // Array format: multiple artifact patterns (searches all workflows) if (Array.isArray(config)) { - return [ - { - workflow: undefined, - artifacts: normalizeArtifactPatterns(config), - }, - ]; + return [{ workflow: undefined, artifacts: normalizeArtifactPatterns(config) }]; } - // Object format: workflow-scoped patterns const filters: NormalizedArtifactFilter[] = []; for (const [workflowPattern, artifactPatterns] of Object.entries(config)) { filters.push({ @@ -411,14 +398,11 @@ export class GitHubArtifactProvider extends BaseArtifactProvider { }); for (const artifact of response.data.artifacts) { - // Check if this artifact matches any filter for (const filter of filters) { - // If filter has a workflow pattern, check if this run matches it if (filter.workflow && !filter.workflow.test(workflowName)) { continue; } - // Check if artifact name matches any of the artifact patterns const matches = filter.artifacts.some(pattern => pattern.test(artifact.name) ); @@ -427,7 +411,7 @@ export class GitHubArtifactProvider extends BaseArtifactProvider { `Artifact "${artifact.name}" matches filter from workflow "${workflowName}"` ); matchingArtifacts.push(artifact); - break; // Don't add the same artifact twice + break; } } } @@ -489,7 +473,6 @@ export class GitHubArtifactProvider extends BaseArtifactProvider { } of ${MAX_TRIES})` ); - // Get workflow runs for the commit const allRuns = await this.getWorkflowRunsForCommit(revision); if (allRuns.length === 0) { this.logger.debug(`No workflow runs found for commit ${revision}`); @@ -507,13 +490,11 @@ export class GitHubArtifactProvider extends BaseArtifactProvider { ); } - // Filter workflow runs by patterns const filteredRuns = this.filterWorkflowRuns(allRuns, filters); this.logger.debug( `${filteredRuns.length} of ${allRuns.length} workflow runs match filters` ); - // Get artifacts from filtered workflow runs const matchingArtifacts = await this.getArtifactsFromWorkflowRuns( filteredRuns, filters @@ -535,14 +516,12 @@ export class GitHubArtifactProvider extends BaseArtifactProvider { ); } - // Download artifacts in parallel this.logger.debug( `Downloading ${matchingArtifacts.length} artifacts in parallel...` ); return await this.downloadArtifactsInParallel(matchingArtifacts); } - // This should not be reached, but TypeScript needs a return throw new Error( `Failed to fetch artifacts for revision "${revision}" (tries: ${MAX_TRIES})` ); @@ -554,18 +533,16 @@ export class GitHubArtifactProvider extends BaseArtifactProvider { protected async doListArtifactsForRevision( revision: string ): Promise { - // Check if artifacts config is present const artifactsConfig = this.config.artifacts as | GitHubArtifactsConfig | undefined; const filters = normalizeArtifactsConfig(artifactsConfig); if (filters.length > 0) { - // Use new workflow-based approach return await this.doListArtifactsWithFilters(revision, filters); } - // Legacy behavior: artifact.name === revision SHA + // Legacy: artifact.name === revision SHA const foundArtifact = await this.getRevisionArtifact(revision); this.logger.debug(`Requesting archive URL from GitHub...`); diff --git a/src/schemas/project_config.ts b/src/schemas/project_config.ts index 7876a8f3..9daaf46d 100644 --- a/src/schemas/project_config.ts +++ b/src/schemas/project_config.ts @@ -88,16 +88,14 @@ export type BaseArtifactProvider = z.infer; const ArtifactPatternsSchema = z.union([z.string(), z.array(z.string())]); /** - * Full artifacts config for GitHub artifact provider: - * - string: single artifact pattern (searches all workflows) - * - array: multiple artifact patterns (searches all workflows) - * - object: workflow-scoped patterns (keys = workflow name/pattern, values = artifact(s)) + * Artifacts config for GitHub artifact provider. + * Accepts string, array of strings, or object mapping workflow names to artifact patterns. */ export const GitHubArtifactsConfigSchema = z .union([ - z.string(), // single artifact pattern - z.array(z.string()), // array of artifact patterns - z.record(z.string(), ArtifactPatternsSchema), // workflow → artifact(s) + z.string(), + z.array(z.string()), + z.record(z.string(), ArtifactPatternsSchema), ]) .optional(); From 467a4b3f4dd200c78340a66025d287d3db0bce4f Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Tue, 13 Jan 2026 15:20:35 +0000 Subject: [PATCH 3/5] refactor: Address PR feedback - Export ArtifactPatternsSchema from schema - Move patternToRegexp and escapeRegex to utils/filters.ts - Restructure getArtifactsFromWorkflowRuns to pre-compute applicable patterns per workflow, eliminating redundant filter checks - Use Set to track seen artifact IDs instead of break statement - Implement two-phase download pipeline: download all in parallel, then unpack all in parallel for better I/O throughput - Add tests for new filter utility functions --- src/artifact_providers/github.ts | 187 ++++++++++++++++------------ src/schemas/project_config.ts | 2 +- src/utils/__tests__/filters.test.ts | 48 ++++++- src/utils/filters.ts | 19 +++ 4 files changed, 172 insertions(+), 84 deletions(-) diff --git a/src/artifact_providers/github.ts b/src/artifact_providers/github.ts index 05244e00..7e524976 100644 --- a/src/artifact_providers/github.ts +++ b/src/artifact_providers/github.ts @@ -18,9 +18,12 @@ import { } from '../utils/files'; import { extractZipArchive } from '../utils/system'; import { sleep } from '../utils/async'; -import { stringToRegexp } from '../utils/filters'; +import { patternToRegexp } from '../utils/filters'; import { GitHubArtifactsConfig } from '../schemas/project_config'; +// Re-export for backward compatibility with tests +export { patternToRegexp } from '../utils/filters'; + const MAX_TRIES = 3; const MILLISECONDS = 1000; const ARTIFACTS_POLLING_INTERVAL = 10 * MILLISECONDS; @@ -36,24 +39,10 @@ export type WorkflowRun = * Normalized artifact filter structure */ export interface NormalizedArtifactFilter { - /** Optional workflow name pattern to match (if undefined, matches all workflows) */ workflow?: RegExp; - /** Artifact name patterns to match */ artifacts: RegExp[]; } -/** - * Converts a string to a RegExp if it's wrapped in slashes, otherwise creates an exact match RegExp - */ -export function patternToRegexp(pattern: string): RegExp { - if (pattern.startsWith('/') && pattern.lastIndexOf('/') > 0) { - return stringToRegexp(pattern); - } - // Exact match (escape special regex characters) - const escaped = pattern.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); - return new RegExp(`^${escaped}$`); -} - /** * Normalizes artifact patterns from string/array format to array of RegExp */ @@ -243,46 +232,12 @@ export class GitHubArtifactProvider extends BaseArtifactProvider { } /** - * Downloads and unpacks a GitHub artifact in a temp folder - * @param archiveResponse + * Downloads and unpacks a single GitHub artifact */ - private async downloadAndUnpackArtifacts( - url: string - ): Promise { - const artifacts: RemoteArtifact[] = []; - await withTempFile(async tempFilepath => { - const response = await fetch(url); - if (!response.ok) { - throw new Error( - `Unexpected HTTP response from ${url}: ${response.status} (${response.statusText})` - ); - } - await new Promise((resolve, reject) => - response.body - .pipe(fs.createWriteStream(tempFilepath)) - .on('finish', () => resolve()) - .on('error', reject) - ); - this.logger.info(`Finished downloading.`); - - await withTempDir(async tmpDir => { - this.logger.debug(`Extracting "${tempFilepath}" to "${tmpDir}"...`); - await extractZipArchive(tempFilepath, tmpDir); - (await scan(tmpDir)).forEach(file => { - artifacts.push({ - filename: path.basename(file), - mimeType: detectContentType(file), - storedFile: { - downloadFilepath: file, - filename: path.basename(file), - size: fs.lstatSync(file).size, - }, - } as RemoteArtifact); - }); - }, false); - }); - - return artifacts; + private async downloadAndUnpackArtifacts(url: string): Promise { + const tempFile = await this.downloadToTempFile(url); + this.logger.info(`Finished downloading.`); + return this.unpackArtifact(tempFile); } /** @@ -367,12 +322,24 @@ export class GitHubArtifactProvider extends BaseArtifactProvider { }); } + /** + * Gets the artifact patterns applicable to a workflow run + */ + private getApplicablePatterns( + workflowName: string, + filters: NormalizedArtifactFilter[] + ): RegExp[] { + const patterns: RegExp[] = []; + for (const filter of filters) { + if (!filter.workflow || filter.workflow.test(workflowName)) { + patterns.push(...filter.artifacts); + } + } + return patterns; + } + /** * Gets artifacts from workflow runs and filters them by patterns - * - * @param runs Array of workflow runs - * @param filters Array of normalized filters - * @returns Array of matching artifacts */ protected async getArtifactsFromWorkflowRuns( runs: WorkflowRun[], @@ -380,9 +347,15 @@ export class GitHubArtifactProvider extends BaseArtifactProvider { ): Promise { const { repoName: repo, repoOwner: owner } = this.config; const matchingArtifacts: ArtifactItem[] = []; + const seenArtifactIds = new Set(); for (const run of runs) { const workflowName = run.name ?? ''; + const patterns = this.getApplicablePatterns(workflowName, filters); + if (patterns.length === 0) { + continue; + } + this.logger.debug( `Fetching artifacts for workflow run "${workflowName}" (ID: ${run.id})` ); @@ -398,21 +371,16 @@ export class GitHubArtifactProvider extends BaseArtifactProvider { }); for (const artifact of response.data.artifacts) { - for (const filter of filters) { - if (filter.workflow && !filter.workflow.test(workflowName)) { - continue; - } - - const matches = filter.artifacts.some(pattern => - pattern.test(artifact.name) + if (seenArtifactIds.has(artifact.id)) { + continue; + } + const matches = patterns.some(pattern => pattern.test(artifact.name)); + if (matches) { + this.logger.debug( + `Artifact "${artifact.name}" matches filter from workflow "${workflowName}"` ); - if (matches) { - this.logger.debug( - `Artifact "${artifact.name}" matches filter from workflow "${workflowName}"` - ); - matchingArtifacts.push(artifact); - break; - } + seenArtifactIds.add(artifact.id); + matchingArtifacts.push(artifact); } } @@ -427,31 +395,88 @@ export class GitHubArtifactProvider extends BaseArtifactProvider { } /** - * Downloads multiple artifacts in parallel and unpacks them - * - * @param artifactItems Array of artifact items to download - * @returns Array of remote artifacts from all downloaded zip files + * Downloads an artifact to a temp file + */ + private async downloadToTempFile(url: string): Promise { + return withTempFile( + async tempFilepath => { + const response = await fetch(url); + if (!response.ok) { + throw new Error( + `Unexpected HTTP response from ${url}: ${response.status} (${response.statusText})` + ); + } + await new Promise((resolve, reject) => + response.body + .pipe(fs.createWriteStream(tempFilepath)) + .on('finish', () => resolve()) + .on('error', reject) + ); + return tempFilepath; + }, + false // Don't cleanup - we'll handle it after unpacking + ); + } + + /** + * Unpacks a downloaded zip file and returns the artifacts + */ + private async unpackArtifact(tempFilepath: string): Promise { + const artifacts: RemoteArtifact[] = []; + await withTempDir(async tmpDir => { + this.logger.debug(`Extracting "${tempFilepath}" to "${tmpDir}"...`); + await extractZipArchive(tempFilepath, tmpDir); + (await scan(tmpDir)).forEach(file => { + artifacts.push({ + filename: path.basename(file), + mimeType: detectContentType(file), + storedFile: { + downloadFilepath: file, + filename: path.basename(file), + size: fs.lstatSync(file).size, + }, + } as RemoteArtifact); + }); + }, false); + + // Clean up the temp zip file + fs.unlinkSync(tempFilepath); + return artifacts; + } + + /** + * Downloads and unpacks multiple artifacts using a two-phase pipeline: + * 1. Download all artifacts in parallel + * 2. Unpack all artifacts in parallel */ protected async downloadArtifactsInParallel( artifactItems: ArtifactItem[] ): Promise { const limit = pLimit(DOWNLOAD_CONCURRENCY); - const allArtifacts: RemoteArtifact[] = []; + // Phase 1: Download all artifacts in parallel + this.logger.debug(`Downloading ${artifactItems.length} artifacts...`); const downloadPromises = artifactItems.map(item => limit(async () => { this.logger.debug(`Downloading artifact "${item.name}"...`); const url = await this.getArchiveDownloadUrl(item); - const artifacts = await this.downloadAndUnpackArtifacts(url); - return artifacts; + return this.downloadToTempFile(url); }) ); + const tempFiles = await Promise.all(downloadPromises); + this.logger.info(`Downloaded ${tempFiles.length} artifacts.`); + + // Phase 2: Unpack all artifacts in parallel + this.logger.debug(`Unpacking ${tempFiles.length} artifacts...`); + const unpackPromises = tempFiles.map(tempFile => + limit(() => this.unpackArtifact(tempFile)) + ); + const results = await Promise.all(unpackPromises); - const results = await Promise.all(downloadPromises); + const allArtifacts: RemoteArtifact[] = []; for (const artifacts of results) { allArtifacts.push(...artifacts); } - return allArtifacts; } diff --git a/src/schemas/project_config.ts b/src/schemas/project_config.ts index 9daaf46d..4cea6a89 100644 --- a/src/schemas/project_config.ts +++ b/src/schemas/project_config.ts @@ -85,7 +85,7 @@ export type BaseArtifactProvider = z.infer; /** * Artifact pattern(s) for a single workflow - can be a single string or array of strings */ -const ArtifactPatternsSchema = z.union([z.string(), z.array(z.string())]); +export const ArtifactPatternsSchema = z.union([z.string(), z.array(z.string())]); /** * Artifacts config for GitHub artifact provider. diff --git a/src/utils/__tests__/filters.test.ts b/src/utils/__tests__/filters.test.ts index 260e5b2d..d29722a5 100644 --- a/src/utils/__tests__/filters.test.ts +++ b/src/utils/__tests__/filters.test.ts @@ -1,5 +1,4 @@ -import { vi, type Mock, type MockInstance, type Mocked, type MockedFunction } from 'vitest'; -import { stringToRegexp } from '../filters'; +import { stringToRegexp, escapeRegex, patternToRegexp } from '../filters'; describe('stringToRegexp', () => { test('converts string without special characters', () => { @@ -31,3 +30,48 @@ describe('stringToRegexp', () => { } }); }); + +describe('escapeRegex', () => { + test('escapes special regex characters', () => { + expect(escapeRegex('hello.world')).toBe('hello\\.world'); + expect(escapeRegex('file[0].txt')).toBe('file\\[0\\]\\.txt'); + expect(escapeRegex('a+b*c?')).toBe('a\\+b\\*c\\?'); + expect(escapeRegex('(foo|bar)')).toBe('\\(foo\\|bar\\)'); + expect(escapeRegex('$100')).toBe('\\$100'); + expect(escapeRegex('^start')).toBe('\\^start'); + }); + + test('leaves normal characters unchanged', () => { + expect(escapeRegex('hello-world')).toBe('hello-world'); + expect(escapeRegex('foo_bar')).toBe('foo_bar'); + }); +}); + +describe('patternToRegexp', () => { + test('converts regex string to RegExp', () => { + const result = patternToRegexp('/^build-.*$/'); + expect(result).toBeInstanceOf(RegExp); + expect(result.test('build-linux')).toBe(true); + expect(result.test('build-macos')).toBe(true); + expect(result.test('test-linux')).toBe(false); + }); + + test('converts regex string with modifiers', () => { + const result = patternToRegexp('/BUILD/i'); + expect(result.test('build')).toBe(true); + expect(result.test('BUILD')).toBe(true); + }); + + test('converts exact string to exact match RegExp', () => { + const result = patternToRegexp('build'); + expect(result.test('build')).toBe(true); + expect(result.test('build-linux')).toBe(false); + expect(result.test('mybuild')).toBe(false); + }); + + test('escapes special characters in exact match', () => { + const result = patternToRegexp('output.tar.gz'); + expect(result.test('output.tar.gz')).toBe(true); + expect(result.test('outputXtarXgz')).toBe(false); + }); +}); diff --git a/src/utils/filters.ts b/src/utils/filters.ts index 72dc9d0b..8046d2d1 100644 --- a/src/utils/filters.ts +++ b/src/utils/filters.ts @@ -24,3 +24,22 @@ export function stringToRegexp(str: string): RegExp { const regexpModifiers = str.slice(lastSlash + 1); return new RegExp(regexpString, regexpModifiers); } + +/** + * Escapes special regex characters in a string for use in RegExp + */ +export function escapeRegex(str: string): string { + return str.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); +} + +/** + * Converts a pattern string to a RegExp. + * If wrapped in slashes (e.g., /^foo.*$/), parses as regex. + * Otherwise, creates an exact match pattern. + */ +export function patternToRegexp(pattern: string): RegExp { + if (pattern.startsWith('/') && pattern.lastIndexOf('/') > 0) { + return stringToRegexp(pattern); + } + return new RegExp(`^${escapeRegex(pattern)}$`); +} From 0bdf5a1d9f2b81236af4bdc6e4182a5aea3f17d6 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Tue, 13 Jan 2026 15:29:20 +0000 Subject: [PATCH 4/5] refactor: Address additional PR feedback - Use ArtifactPatternsSchema in GitHubArtifactsConfigSchema union - Remove re-export of patternToRegexp from github.ts - Update tests to import patternToRegexp from utils/filters.ts --- src/artifact_providers/__tests__/github.test.ts | 2 +- src/artifact_providers/github.ts | 3 --- src/schemas/project_config.ts | 3 +-- 3 files changed, 2 insertions(+), 6 deletions(-) diff --git a/src/artifact_providers/__tests__/github.test.ts b/src/artifact_providers/__tests__/github.test.ts index 6311c742..69b68e75 100644 --- a/src/artifact_providers/__tests__/github.test.ts +++ b/src/artifact_providers/__tests__/github.test.ts @@ -16,9 +16,9 @@ import { lazyRequest, lazyRequestCallback, normalizeArtifactsConfig, - patternToRegexp, NormalizedArtifactFilter, } from '../github'; +import { patternToRegexp } from '../../utils/filters'; import { sleep } from '../../utils/async'; class TestGitHubArtifactProvider extends GitHubArtifactProvider { diff --git a/src/artifact_providers/github.ts b/src/artifact_providers/github.ts index 7e524976..26eb986e 100644 --- a/src/artifact_providers/github.ts +++ b/src/artifact_providers/github.ts @@ -21,9 +21,6 @@ import { sleep } from '../utils/async'; import { patternToRegexp } from '../utils/filters'; import { GitHubArtifactsConfig } from '../schemas/project_config'; -// Re-export for backward compatibility with tests -export { patternToRegexp } from '../utils/filters'; - const MAX_TRIES = 3; const MILLISECONDS = 1000; const ARTIFACTS_POLLING_INTERVAL = 10 * MILLISECONDS; diff --git a/src/schemas/project_config.ts b/src/schemas/project_config.ts index 4cea6a89..0180562d 100644 --- a/src/schemas/project_config.ts +++ b/src/schemas/project_config.ts @@ -93,8 +93,7 @@ export const ArtifactPatternsSchema = z.union([z.string(), z.array(z.string())]) */ export const GitHubArtifactsConfigSchema = z .union([ - z.string(), - z.array(z.string()), + ArtifactPatternsSchema, z.record(z.string(), ArtifactPatternsSchema), ]) .optional(); From cf03ca1f5f5e80c8be7313cc6622967ff90b6ed8 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Fri, 30 Jan 2026 21:38:10 +0000 Subject: [PATCH 5/5] fix(github): Ensure temp files are cleaned up even if artifact unpacking fails - Use Promise.allSettled instead of Promise.all for unpacking - Clean up temp files for failed unpacks to prevent resource leaks - Aggregate and report all unpacking errors Addresses review feedback to prevent disk space exhaustion from orphaned temporary files when unpacking operations fail. Co-authored-by: burak.kaya --- src/artifact_providers/github.ts | 35 +++++++++++++++++++++++++++++--- 1 file changed, 32 insertions(+), 3 deletions(-) diff --git a/src/artifact_providers/github.ts b/src/artifact_providers/github.ts index 26eb986e..51026f57 100644 --- a/src/artifact_providers/github.ts +++ b/src/artifact_providers/github.ts @@ -464,16 +464,45 @@ export class GitHubArtifactProvider extends BaseArtifactProvider { this.logger.info(`Downloaded ${tempFiles.length} artifacts.`); // Phase 2: Unpack all artifacts in parallel + // Use allSettled to ensure we can clean up all temp files even if some unpacking fails this.logger.debug(`Unpacking ${tempFiles.length} artifacts...`); const unpackPromises = tempFiles.map(tempFile => limit(() => this.unpackArtifact(tempFile)) ); - const results = await Promise.all(unpackPromises); + const results = await Promise.allSettled(unpackPromises); + // Collect successful unpacks and clean up failed ones const allArtifacts: RemoteArtifact[] = []; - for (const artifacts of results) { - allArtifacts.push(...artifacts); + const errors: Error[] = []; + + for (let i = 0; i < results.length; i++) { + const result = results[i]; + const tempFile = tempFiles[i]; + + if (result.status === 'fulfilled') { + allArtifacts.push(...result.value); + } else { + errors.push(result.reason); + // Clean up temp file if unpacking failed + try { + if (fs.existsSync(tempFile)) { + fs.unlinkSync(tempFile); + this.logger.debug(`Cleaned up temp file after failed unpack: ${tempFile}`); + } + } catch (cleanupError) { + this.logger.warn(`Failed to clean up temp file ${tempFile}:`, cleanupError); + } + } } + + // If any unpacking failed, throw an error with details + if (errors.length > 0) { + const errorMessages = errors.map(e => e.message).join('; '); + throw new Error( + `Failed to unpack ${errors.length} of ${tempFiles.length} artifacts: ${errorMessages}` + ); + } + return allArtifacts; }