diff --git a/.github/workflows/checks.yml b/.github/workflows/checks.yml index b42f8dc17..917b037bd 100644 --- a/.github/workflows/checks.yml +++ b/.github/workflows/checks.yml @@ -49,3 +49,6 @@ jobs: - name: Test the PR evaluator run: pnpm test:evaluator + + - name: Test the signed-commit helpers + run: pnpm test:api-commits diff --git a/.github/workflows/wizard-ci.yml b/.github/workflows/wizard-ci.yml index 80a587a13..033be7769 100644 --- a/.github/workflows/wizard-ci.yml +++ b/.github/workflows/wizard-ci.yml @@ -990,16 +990,16 @@ jobs: if: always() && (needs.notify-start.outputs.thread_ts != '' || needs.discover.outputs.input_notify_pr == 'true') id: job-id run: | - # Fetch the current job ID from the GitHub API - JOB_ID=$(curl -s -H "Authorization: Bearer $APP_TOKEN" \ - "$GITHUB_API_URL/repos/$GITHUB_REPO/actions/runs/$GITHUB_RUN_ID/jobs" \ - | jq -r --arg app "$APP" '.jobs[] | select(.name | contains($app)) | .id' | head -1) + # Fetch the current job ID from the GitHub API. A full run has more + # jobs than one page holds, so page through all of them, and match + # the name exactly: `next-stripe` is a prefix of `next-stripe-declined`. + JOB_ID=$(gh api --paginate "repos/$GITHUB_REPO/actions/runs/$GITHUB_RUN_ID/jobs?per_page=100" \ + | jq -r --arg name "wizard-ci ($APP)" '.jobs[] | select(.name == $name) | .id' | head -1) echo "job_id=$JOB_ID" >> $GITHUB_OUTPUT echo "Job ID: $JOB_ID" env: - APP_TOKEN: ${{ steps.app-token.outputs.token }} + GH_TOKEN: ${{ steps.app-token.outputs.token }} APP: ${{ matrix.app }} - GITHUB_API_URL: ${{ github.api_url }} GITHUB_REPO: ${{ github.repository }} GITHUB_RUN_ID: ${{ github.run_id }} diff --git a/package.json b/package.json index ed8bff2ab..6d78bf188 100644 --- a/package.json +++ b/package.json @@ -13,6 +13,7 @@ "mcp-stub": "tsx services/mcp-stub/cli.ts", "mcp-stub:record": "tsx services/mcp-stub/record.ts", "test:evaluator": "tsx --test services/pr-evaluator/evaluator.test.ts", + "test:api-commits": "tsx --test services/github/api-commits.test.ts", "test:mcp-stub": "tsx --test services/mcp-stub/mcp-stub.test.ts", "test:warehouse-checks": "tsx --test services/wizard-ci/warehouse-checks.test.ts", "test:feature-flag-checks": "tsx --test services/wizard-ci/feature-flag-checks.test.ts", diff --git a/services/github/api-commits.test.ts b/services/github/api-commits.test.ts index bc289b6cd..822ab5e65 100644 --- a/services/github/api-commits.test.ts +++ b/services/github/api-commits.test.ts @@ -4,7 +4,7 @@ import { mkdirSync, mkdtempSync, symlinkSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import test from "node:test"; -import { batchFileChanges, collectFileChanges } from "./api-commits.js"; +import { batchFileChanges, collectFileChanges, commitBatch } from "./api-commits.js"; test("large snapshot bundles retain every file in bounded commit requests", () => { const additions = Array.from({ length: 86 }, (_, i) => ({ @@ -35,3 +35,48 @@ test("symlinks in the working tree are skipped instead of read", () => { const { additions } = collectFileChanges({ repoRoot, relativePath: "app" }); assert.deepEqual(additions.map((file) => file.path).sort(), ["app/env/lib/site.py", "app/main.py"]); }); + +function fakeOctokit(responses: Array<() => unknown>, branchHead: string) { + let calls = 0; + return { + get calls() { + return calls; + }, + graphql: (async () => { + const next = responses[calls++]; + return next(); + }) as never, + rest: { git: { getRef: (async () => ({ data: { object: { sha: branchHead } } })) as never } }, + }; +} + +const httpError = (status: number) => Object.assign(new Error(`HTTP ${status}`), { status }); +const target = { repoOwner: "PostHog", repoName: "wizard-workbench", branch: "ci/x", headOid: "base" }; +const landed = { createCommitOnBranch: { commit: { oid: "new", url: "https://example.test/new" } } }; + +test("a 504 that did not write the commit is retried", async () => { + const octokit = fakeOctokit([() => Promise.reject(httpError(504)), () => landed], "base"); + const commit = await commitBatch(octokit, target, {}, 0); + assert.equal(commit.oid, "new"); + assert.equal(octokit.calls, 2); +}); + +test("a 504 after GitHub wrote the commit returns that commit instead of resending", async () => { + const octokit = fakeOctokit([() => Promise.reject(httpError(504))], "written-by-github"); + const commit = await commitBatch(octokit, target, {}, 0); + assert.equal(commit.oid, "written-by-github"); + assert.equal(octokit.calls, 1); +}); + +test("a 4xx from createCommitOnBranch is not retried", async () => { + const octokit = fakeOctokit([() => Promise.reject(httpError(422))], "base"); + await assert.rejects(commitBatch(octokit, target, {}, 0), /HTTP 422/); + assert.equal(octokit.calls, 1); +}); + +test("a commit that keeps returning 5xx gives up after three attempts", async () => { + const fail = () => Promise.reject(httpError(502)); + const octokit = fakeOctokit([fail, fail, fail], "base"); + await assert.rejects(commitBatch(octokit, target, {}, 0), /HTTP 502/); + assert.equal(octokit.calls, 3); +}); diff --git a/services/github/api-commits.ts b/services/github/api-commits.ts index 7508dc004..1ce12b009 100644 --- a/services/github/api-commits.ts +++ b/services/github/api-commits.ts @@ -256,7 +256,7 @@ export async function createSignedCommit(opts: ApiCommitOptions): Promise(CREATE_COMMIT_MUTATION, { + const commit = await commitBatch(octokit, { repoOwner, repoName, branch, headOid }, { input: { branch: { repositoryNameWithOwner: `${repoOwner}/${repoName}`, @@ -273,13 +273,62 @@ export async function createSignedCommit(opts: ApiCommitOptions): Promise & { rest: { git: Pick } }; + +interface BatchTarget { + repoOwner: string; + repoName: string; + branch: string; + headOid: string; +} + +const COMMIT_ATTEMPTS = 3; + +/** + * Run one createCommitOnBranch mutation, retrying GitHub's 5xx responses. + * + * A 502 or 504 can arrive after GitHub already wrote the commit, or while the + * write is still landing. Resending the same mutation would then fail on + * `expectedHeadOid`, so after each 5xx wait, then check the branch: if it + * moved off `headOid`, that commit is ours. + */ +export async function commitBatch( + octokit: CommitOctokit, + target: BatchTarget, + variables: Record, + retryDelayMs = 2_000, +): Promise<{ oid: string; url: string }> { + const { repoOwner, repoName, branch, headOid } = target; + for (let attempt = 1; ; attempt++) { + try { + const response = await octokit.graphql(CREATE_COMMIT_MUTATION, variables); + return response.createCommitOnBranch.commit; + } catch (error) { + const status = (error as { status?: number }).status ?? 0; + if (status < 500) throw error; + console.warn(` createCommitOnBranch returned ${status} (attempt ${attempt}/${COMMIT_ATTEMPTS})`); + await new Promise((resolve) => setTimeout(resolve, retryDelayMs * attempt)); + const landed = await branchHead(octokit, target).catch(() => null); + if (landed && landed !== headOid) { + return { oid: landed, url: `https://github.com/${repoOwner}/${repoName}/commit/${landed}` }; + } + if (attempt >= COMMIT_ATTEMPTS) throw error; + } + } +} + +async function branchHead(octokit: CommitOctokit, { repoOwner, repoName, branch }: BatchTarget): Promise { + const { data } = await octokit.rest.git.getRef({ owner: repoOwner, repo: repoName, ref: `heads/${branch}` }); + return data.object.sha; +} + // ============================================================================ // Repo identification // ============================================================================ diff --git a/services/wizard-ci/e2e.ts b/services/wizard-ci/e2e.ts index 02c33bf2b..4ad5a607c 100644 --- a/services/wizard-ci/e2e.ts +++ b/services/wizard-ci/e2e.ts @@ -69,7 +69,9 @@ const INJECTED_CREDENTIALS: Record = { E2E_PG_DATABASE: "e2e_fixture_db", E2E_PG_USER: "e2e_fixture_user", E2E_PG_PASSWORD: "e2e-fixture-placeholder-password", - E2E_STRIPE_API_KEY: "sk_test_e2efixtureplaceholder000000", + // The data-warehouse-source skill asks for a restricted key and the agent + // refuses an `sk_` secret key, so the fixture has to look like one. + E2E_STRIPE_API_KEY: "rk_live_e2e_fixture_placeholder_0000", // The catch-all the wizard profile routes every other credential-shaped // question to — a Hugging Face access token, a bare `api_key`. Without it // those questions reach the `e2e` sentinel, and the create they feed fails. @@ -142,6 +144,21 @@ export function snapsDirFor(app: string): string { return `/tmp/wizard-e2e-${basename(app)}-snaps`; } +/** + * The seeded data-source step runs inside the install program, whose e2e + * profile has no `askAnswers`. Without the warehouse program's credential + * rules every Stripe question falls to the sentinel and nothing is created, + * so hand the child those rules through `E2E_ANSWERS_FILE`. + */ +function writeSeededAnswersFile(repo: string, app: string): string { + const profile = JSON.parse( + readFileSync(join(repo, "src", "programs", "warehouse-source", "test", "e2e.json"), "utf8"), + ) as { profile?: { askAnswers?: unknown[] } }; + const file = `/tmp/wizard-e2e-${basename(app)}-answers.json`; + writeFileSync(file, JSON.stringify(profile.profile?.askAnswers ?? [])); + return file; +} + /** * Where the stub MCP writes the run's journal — the evidence that a source was * really created. The workflow uploads this file, so keep the path derivable. @@ -387,6 +404,7 @@ export async function runE2e(opts: E2eOptions): Promise { childEnv.WIZARD_CI_FLAG_OVERRIDES, SEEDED_FLAG_OVERRIDES, ); + childEnv.E2E_ANSWERS_FILE = writeSeededAnswersFile(repo, app); } console.log(` stub mcp: ${stub.url} journal: ${journalPath}`); if (expect.attemptedFailOk.length) { diff --git a/services/wizard-ci/warehouse-checks.test.ts b/services/wizard-ci/warehouse-checks.test.ts index 94b11ba8e..f915fb643 100644 --- a/services/wizard-ci/warehouse-checks.test.ts +++ b/services/wizard-ci/warehouse-checks.test.ts @@ -901,6 +901,13 @@ describe("report written", () => { assert.equal(c.ok, true); assert.match(c.detail, /not expected/); }); + + it("is not expected when a seeded run's program declares no report file", () => { + const checks = grade(expectation({ seeded: true }), result({ reportFile: undefined })); + const c = named(checks, "report written"); + assert.equal(c.ok, true); + assert.match(c.detail, /not expected/); + }); }); describe("abort", () => { diff --git a/services/wizard-ci/warehouse-checks.ts b/services/wizard-ci/warehouse-checks.ts index 3a42299bb..ec8146dfe 100644 --- a/services/wizard-ci/warehouse-checks.ts +++ b/services/wizard-ci/warehouse-checks.ts @@ -809,6 +809,12 @@ export function warehouseChecks(evidence: WarehouseEvidence): Check[] { checks.push( add("report written", true, "not expected — this run aborts before the report"), ); + } else if (expect.seeded && !result.reportFile) { + // The seeded data-source step runs inside the install program, which + // writes its own report and declares no warehouse report file. + checks.push( + add("report written", true, "not expected — the seeded step declares no report file"), + ); } else { const file = result.reportFile; const missingFromReport = [...succeeded].filter(