From 9f03955d858fed3f056c1bbf297b27dc5ee575a7 Mon Sep 17 00:00:00 2001 From: jbeckwith-oai <269036200+jbeckwith-oai@users.noreply.github.com> Date: Mon, 26 Oct 2026 23:34:18 +0000 Subject: [PATCH] chore(api): preserve Chat and chunked-upload helper interfaces Castiron-Internal-PR: https://github.com/openai/openai-python-internal/pull/354 Castiron-Source-SHA: 8a7d93e1f9b41bf88d48280f7917b2850f9b1f14 Castiron-Public-Base-SHA: 06dbb5d019d7b6870b5898bf5df579ca60b61d28 --- .castiron.stats.yml | 10 +- .../castiron-custom-code-comment.yml | 260 ++++--- .github/workflows/castiron-custom-code.yml | 4 +- docs/castiron-generated-code-transition.md | 20 +- scripts/castiron/CUSTOM_CODE.md | 10 + scripts/castiron/custom_code_budget.py | 211 ++++- scripts/castiron/custom_code_policy.py | 121 ++- scripts/castiron/custom_code_report.py | 101 ++- scripts/castiron/custom_code_test_support.py | 15 +- .../castiron/fixtures/github_publisher.cjs | 11 +- scripts/castiron/generated-files.json | 42 +- scripts/castiron/test_custom_code_budget.py | 489 ------------ scripts/castiron/test_custom_code_github.py | 69 +- scripts/castiron/test_custom_code_policy.py | 320 ++++---- .../test_custom_code_policy_source.py | 198 +++++ .../castiron/test_custom_code_publication.py | 258 ++++++- scripts/castiron/test_custom_code_report.py | 18 + .../test_custom_code_review_revocation.py | 238 +++++- .../test_custom_code_status_publication.py | 694 +++++++++++++++++ scripts/castiron/test_custom_code_strict.py | 731 ++++++++++++++++++ src/openai/_client.py | 18 +- 21 files changed, 2987 insertions(+), 851 deletions(-) create mode 100644 scripts/castiron/test_custom_code_policy_source.py create mode 100644 scripts/castiron/test_custom_code_status_publication.py create mode 100644 scripts/castiron/test_custom_code_strict.py diff --git a/.castiron.stats.yml b/.castiron.stats.yml index d3a066ec49..813cdae9c4 100644 --- a/.castiron.stats.yml +++ b/.castiron.stats.yml @@ -1,8 +1,8 @@ schema_version: 1 -generation_id: 491e58ee-627a-4a57-91d7-2d5261cecfe9 +generation_id: a993ea45-7f4f-4d48-9eea-27e2f709ca7c openapi_spec_hash: 1f2d498f08bdd6dacffeb41cee8f44e5 openapi_transformed_spec_hash: 77075630a8655ccbb8d1f790f19f5731 -config_hash: d356dda7ff9ca3fbc614107eb546de9f -codegen_sha: eeef8c0a76fd3885ff1da6b83ed2ba0ef6531414 -codegen_hash: ccac0bcc0ccfd20617a6e93498d22157f63843c1288cf775b7f3d5d393e49358 -public_codegen_sha: 40b2e7c300353a5cc5c8f8992e6616e999f4ecad +config_hash: 7a8181d81e1b9225f8ee6f09171d712e +codegen_sha: 6f2038ca576a63929de017d9145a842c3a09beb1 +codegen_hash: a761b2892108918f3ad49d4929a6b756fa023b3192025bbc2bbcf618dea6c13a +public_codegen_sha: 9fdcc5c60f9247fe11ec25aaa0f486033f738421 diff --git a/.github/workflows/castiron-custom-code-comment.yml b/.github/workflows/castiron-custom-code-comment.yml index 2157f6a87a..aca3ff77fc 100644 --- a/.github/workflows/castiron-custom-code-comment.yml +++ b/.github/workflows/castiron-custom-code-comment.yml @@ -62,7 +62,8 @@ jobs: if (!/^[0-9a-f]{40}$/.test(sha || '')) throw new Error('Invalid policy merge base'); return sha; } - async function requiresHuman(pr, mainSha) { + async function requiresHuman(pr, targetSha, mainSha) { + if (!/^[0-9a-f]{40}$/.test(targetSha || '')) throw new Error('Invalid authenticated target'); if (!/^[0-9a-f]{40}$/.test(mainSha || '')) throw new Error('Invalid authenticated main'); const path = require('node:path'); const fs = require('node:fs'); @@ -84,7 +85,7 @@ jobs: const result = await exec.getExecOutput('python3', ['-I', path.join(root, 'custom_code_policy.py'), 'review-required', '--repository', repo, '--repo', path.join(root, 'objects.git'), - '--base', pr.base.sha, '--head', pr.head.sha, '--main', mainSha], {silent: true}); + '--base', targetSha, '--head', pr.head.sha, '--main', mainSha], {silent: true}); const decision = JSON.parse(result.stdout); if (typeof decision.requires_human !== 'boolean') throw new Error('Invalid review decision'); return decision.requires_human; @@ -93,47 +94,60 @@ jobs: } } const dirtyTargets = new Set(); + const matchesSource = pr => Boolean(run.head_repository?.id && run.head_repository?.full_name && run.head_branch && + pr.head?.ref === run.head_branch && pr.head?.repo?.id === run.head_repository.id && + pr.head?.repo?.full_name === run.head_repository.full_name); // Retry discovery within this job timeout while targets are changing. while (true) { - const candidates = []; + const explicitAssociation = Boolean(run.pull_requests?.length); let pulls = run.pull_requests?.length ? run.pull_requests : await github.paginate( github.rest.repos.listPullRequestsAssociatedWithCommit, {...context.repo, commit_sha: run.head_sha}); - const hasAssociation = pulls.length > 0; - if (!pulls.length) pulls = await github.paginate(github.rest.pulls.list, { - ...context.repo, state: 'open', head: `${run.head_repository.owner.login}:${run.head_branch}`, - }); - for (const item of pulls) { - const {data: pr} = await github.rest.pulls.get({...context.repo, pull_number: item.number}); - if (pr.state !== 'open' || pr.base.ref !== 'next' || pr.base.repo.full_name !== repo || - (item.head?.sha && pr.head.sha !== item.head.sha) || !/^[0-9a-f]{40}$/.test(pr.head.sha)) continue; - if (run.head_sha !== pr.head.sha) { - const {data: commit} = await github.rest.repos.getCommit({...context.repo, ref: run.head_sha}); - if (commit.parents.length !== 2 || commit.parents[1].sha !== pr.head.sha || - !/^[0-9a-f]{40}$/.test(commit.parents[0].sha) || - await mergeBase(commit.parents[0].sha, pr.base.sha) !== commit.parents[0].sha) continue; - // Without a commit association, the live merge ref must identify this PR. - if (!hasAssociation) { - const {data: merge} = await github.rest.git.getRef({...context.repo, ref: `pull/${pr.number}/merge`}); - if (merge.object.sha !== run.head_sha || commit.parents[0].sha !== pr.base.sha) continue; + let hasAssociation = true; + let matching = []; + for (let attempt = 0; attempt < 2; attempt++) { + const candidates = []; + for (const item of pulls) { + const {data: pr} = await github.rest.pulls.get({...context.repo, pull_number: item.number}); + if (pr.state !== 'open' || pr.base.ref !== 'next' || pr.base.repo.full_name !== repo || + (item.head?.sha && pr.head.sha !== item.head.sha) || !/^[0-9a-f]{40}$/.test(pr.head.sha)) continue; + if (!matchesSource(pr)) continue; + if (run.head_sha !== pr.head.sha) { + const {data: commit} = await github.rest.repos.getCommit({...context.repo, ref: run.head_sha}); + if (commit.parents.length !== 2 || commit.parents[1].sha !== pr.head.sha || + !/^[0-9a-f]{40}$/.test(commit.parents[0].sha) || + await mergeBase(commit.parents[0].sha, pr.base.sha) !== commit.parents[0].sha) continue; + // Associated reviews may name an older merge after next advances. + // Branch-only discovery still requires the live merge and base. + if (!hasAssociation) { + const {data: merge} = await github.rest.git.getRef({...context.repo, ref: `pull/${pr.number}/merge`}); + if (merge.object.sha !== run.head_sha || commit.parents[0].sha !== pr.base.sha) continue; + } } + candidates.push(pr); } - candidates.push(pr); + matching = candidates.length === 1 ? candidates : candidates.filter(pr => + Number.isSafeInteger(run.head_repository?.id) && run.head_repository.id > 0 && + pr.head.repo?.id === run.head_repository.id && typeof run.head_branch === 'string' && + run.head_branch.length > 0 && pr.head.ref === run.head_branch); + if (matching.length || explicitAssociation || attempt) break; + if (!run.head_repository?.id || !run.head_repository.full_name || !run.head_branch) return; + pulls = await github.paginate(github.rest.pulls.list, { + ...context.repo, state: 'open', head: `${run.head_repository.owner.login}:${run.head_branch}`, + }); + hasAssociation = false; } - if (!candidates.length) return; // Main, closed, and superseded PR reviews have no next target. - // Commit associations may include other PRs sharing the same head. - const matching = candidates.length === 1 ? candidates : candidates.filter(pr => - Number.isSafeInteger(run.head_repository?.id) && run.head_repository.id > 0 && - pr.head.repo?.id === run.head_repository.id && typeof run.head_branch === 'string' && - run.head_branch.length > 0 && pr.head.ref === run.head_branch); + if (!matching.length) return; if (matching.length !== 1) throw new Error('Review signal identifies ambiguous next PRs'); const pr = matching[0]; const files = await github.paginate(github.rest.pulls.listFiles, {...context.repo, pull_number: pr.number}); if (!files.some(file => file.filename === '.castiron-ratchet.json')) return; + const {data: target} = await github.rest.git.getRef({...context.repo, ref: 'heads/next'}); + const targetSha = target.object.sha; const {data: main} = await github.rest.git.getRef({...context.repo, ref: 'heads/main'}); const mainSha = main.object.sha; let requiresReview; try { - requiresReview = await requiresHuman(pr, mainSha); + requiresReview = await requiresHuman(pr, targetSha, mainSha); } catch (error) { const {data: currentMain} = await github.rest.git.getRef({...context.repo, ref: 'heads/main'}); if (currentMain.object.sha !== mainSha) continue; @@ -145,8 +159,9 @@ jobs: github.rest.git.getRef({...context.repo, ref: 'heads/next'}), mainSha && github.rest.git.getRef({...context.repo, ref: 'heads/main'}), ]); - if (current.head.sha !== pr.head.sha || current.base.sha !== pr.base.sha || - target.object.sha !== pr.base.sha || (main && main.data.object.sha !== mainSha)) continue; + if (current.state !== 'open' || current.head.sha !== pr.head.sha || current.base.ref !== 'next' || + current.base.repo.full_name !== repo || target.object.sha !== targetSha || + (main && main.data.object.sha !== mainSha)) continue; return; } // Read authoritative reviews, including edits: submittedAt alone misses @@ -290,16 +305,23 @@ jobs: if (item.repository.full_name !== repo || item.event !== event || item.head_sha !== sha || item.path.split('@', 1)[0] !== '.github/workflows/castiron-custom-code.yml') continue; if (event === 'pull_request') { + const source = item.head_repository; + if (!source?.id || !source.full_name || !item.head_branch) continue; + const explicitAssociation = Boolean(item.pull_requests?.length); let associated = item.pull_requests?.length ? item.pull_requests : await github.paginate( github.rest.repos.listPullRequestsAssociatedWithCommit, {...context.repo, commit_sha: sha}); - if (!associated.length) associated = await github.paginate(github.rest.pulls.list, { - ...context.repo, state: 'open', head: `${item.head_repository.owner.login}:${item.head_branch}`, - }); const live = []; - for (const reference of associated) { - const {data: pull} = await github.rest.pulls.get({...context.repo, pull_number: reference.number}); - if (pull.state === 'open' && pull.head.sha === sha && pull.base.repo.full_name === repo && - ['main', 'next'].includes(pull.base.ref)) live.push(pull); + for (let attempt = 0; attempt < 2; attempt++) { + for (const reference of associated) { + const {data: pull} = await github.rest.pulls.get({...context.repo, pull_number: reference.number}); + if (pull.state === 'open' && pull.head.sha === sha && pull.base.repo.full_name === repo && + pull.head.repo?.id === source.id && pull.head.repo?.full_name === source.full_name && + pull.head.ref === item.head_branch && ['main', 'next'].includes(pull.base.ref)) live.push(pull); + } + if (live.length || explicitAssociation || attempt) break; + associated = await github.paginate(github.rest.pulls.list, { + ...context.repo, state: 'open', head: `${source.owner.login}:${item.head_branch}`, + }); } // Match the canonical reporter's main preference for shared-head discovery. const main = live.filter(pull => pull.base.ref === 'main'); @@ -315,7 +337,7 @@ jobs: const {data: current} = await github.rest.pulls.get({...context.repo, pull_number: pr.number}); const {data: base} = await github.rest.git.getRef({...context.repo, ref: 'heads/next'}); if (current.state !== 'open' || current.head.sha !== pr.head.sha || current.base.ref !== 'next' || - current.base.repo.full_name !== repo || current.base.sha !== pr.base.sha || base.object.sha !== pr.base.sha || + current.base.repo.full_name !== repo || base.object.sha !== targetSha || JSON.stringify(await queue()) !== JSON.stringify(entries)) continue; // Only normal authenticated reporter runs may restore success. Never infer approval here. for (const run_id of reruns) await github.rest.actions.reRunWorkflow({...context.repo, run_id}); @@ -333,9 +355,11 @@ jobs: pull-requests: read outputs: selected-policy-sha: ${{ steps.select-policy.outputs.policy_sha }} + main-snapshot: ${{ steps.select-policy.outputs.main_snapshot }} selected-base-sha: ${{ steps.select-policy.outputs.base_sha }} selected-head-sha: ${{ steps.select-policy.outputs.head_sha }} selected-branch: ${{ steps.select-policy.outputs.target_branch }} + selected-bootstrap: ${{ steps.select-policy.outputs.bootstrap }} policy-reviews: ${{ steps.select-policy.outputs.policy_reviews }} human-review-rules: ${{ steps.select-policy.outputs.human_review_rules }} number: ${{ steps.report.outputs.number }} @@ -481,9 +505,11 @@ jobs: uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 env: SELECTED_POLICY_SHA: ${{ needs.compute.outputs.selected-policy-sha }} + MAIN_SNAPSHOT: ${{ needs.compute.outputs.main-snapshot }} SELECTED_BASE_SHA: ${{ needs.compute.outputs.selected-base-sha }} SELECTED_HEAD_SHA: ${{ needs.compute.outputs.selected-head-sha }} SELECTED_BRANCH: ${{ needs.compute.outputs.selected-branch }} + SELECTED_BOOTSTRAP: ${{ needs.compute.outputs.selected-bootstrap }} POLICY_REVIEWS: ${{ needs.compute.outputs.policy-reviews }} HUMAN_REVIEW_RULES: ${{ needs.compute.outputs.human-review-rules }} TARGET_BRANCH: ${{ needs.compute.outputs.target-branch }} @@ -505,6 +531,9 @@ jobs: if (run.event !== event.event || run.head_sha !== event.head_sha || run.run_attempt !== event.run_attempt || run.status !== 'completed' || run.path.split('@', 1)[0] !== '.github/workflows/castiron-custom-code.yml' || run.repository.full_name !== `${context.repo.owner}/${context.repo.repo}`) return; + const matchesSource = pr => Boolean(run.head_repository?.id && run.head_repository?.full_name && run.head_branch && + pr.head?.sha === run.head_sha && pr.head?.ref === run.head_branch && + pr.head?.repo?.id === run.head_repository.id && pr.head?.repo?.full_name === run.head_repository.full_name); const head = run.head_sha; if (!/^[0-9a-f]{40}$/.test(head)) throw new Error('Invalid candidate SHA'); const base = process.env.BASE_SHA; @@ -513,16 +542,21 @@ jobs: let branch, number; let fresh = /^[0-9a-f]{40}$/.test(base) && /^[0-9a-f]{40}$/.test(policy) && head === process.env.HEAD_SHA; if (run.event === 'pull_request') { + if (!run.head_repository?.id || !run.head_repository.full_name || !run.head_branch) return; + const explicitAssociation = Boolean(run.pull_requests?.length); let pulls = run.pull_requests.length ? run.pull_requests : await github.paginate( github.rest.repos.listPullRequestsAssociatedWithCommit, {...context.repo, commit_sha: head}); - if (!pulls.length) pulls = await github.paginate(github.rest.pulls.list, { - ...context.repo, state: 'open', head: `${run.head_repository.owner.login}:${run.head_branch}`, - }); const current = []; - for (const pull of pulls) { - const {data: pr} = await github.rest.pulls.get({...context.repo, pull_number: pull.number}); - if (pr.state === 'open' && pr.head.sha === head && - ['main', 'next'].includes(pr.base.ref) && pr.base.repo.full_name === `${context.repo.owner}/${context.repo.repo}`) current.push(pr); + for (let attempt = 0; attempt < 2; attempt++) { + for (const pull of pulls) { + const {data: pr} = await github.rest.pulls.get({...context.repo, pull_number: pull.number}); + if (pr.state === 'open' && matchesSource(pr) && + ['main', 'next'].includes(pr.base.ref) && pr.base.repo.full_name === `${context.repo.owner}/${context.repo.repo}`) current.push(pr); + } + if (current.length || explicitAssociation || attempt) break; + pulls = await github.paginate(github.rest.pulls.list, { + ...context.repo, state: 'open', head: `${run.head_repository.owner.login}:${run.head_branch}`, + }); } // Match compute's preference for required main checks when // fallback discovery finds an advisory next PR for the same head. @@ -541,6 +575,8 @@ jobs: process.env.SELECTED_BASE_SHA === base && process.env.SELECTED_HEAD_SHA === head && process.env.SELECTED_POLICY_SHA === policy; fresh = fresh && evaluatedBranch === branch; + const bootstrap = process.env.SELECTED_BOOTSTRAP; + fresh = fresh && (bootstrap === 'false' || (branch === 'next' && bootstrap === 'true')); const url = `${context.serverUrl}/${context.repo.owner}/${context.repo.repo}/actions/runs/${context.runId}`; // Concurrency serializes writes, but does not order runs or retries. // Keep an older evaluation from overwriting a newer result for this head. @@ -557,39 +593,6 @@ jobs: const different = previous.findIndex((value, index) => value !== order[index]); if (different !== -1 && previous[different] > order[different]) return; } - // Recheck after pagination, immediately before attaching statuses. - if (run.event === 'pull_request') { - const {data: pr} = await github.rest.pulls.get({...context.repo, pull_number: number}); - if (pr.state !== 'open' || pr.head.sha !== head || pr.base.ref !== branch || - pr.base.repo.full_name !== `${context.repo.owner}/${context.repo.repo}`) return; - } else { - // A removed/rebuilt queue entry must not receive a passing result. - let cursor = null, member = false; - do { - const response = await github.graphql(`query($owner:String!,$name:String!,$branch:String!,$cursor:String) { - repository(owner:$owner,name:$name) { mergeQueue(branch:$branch) { - entries(first:100,after:$cursor) { - nodes { headCommit { oid } } - pageInfo { hasNextPage endCursor } - } - } } - }`, {owner: context.repo.owner, name: context.repo.repo, branch, cursor}); - const entries = response.repository.mergeQueue?.entries; - if (!entries) break; - member = entries.nodes.some(entry => entry.headCommit?.oid === head); - if (member || !entries.pageInfo.hasNextPage) break; - const next = entries.pageInfo.endCursor; - if (!next || next === cursor) throw new Error('Invalid queue pagination'); - cursor = next; - } while (true); - const {data: target} = await github.rest.git.getRef({...context.repo, ref: `heads/${branch}`}); - fresh = fresh && base === target.object.sha && (branch === 'next' || policy === target.object.sha); - fresh = fresh && member; - } - if (branch === 'next' && run.event === 'pull_request') { - const {data: target} = await github.rest.git.getRef({...context.repo, ref: 'heads/next'}); - fresh = fresh && base === target.object.sha; - } // Selection is not a lasting approval. Recheck live reviews after // compute and immediately before attaching required statuses. if (branch === 'next') { @@ -644,6 +647,52 @@ jobs: } } catch { fresh = false; } } + // Recheck after pagination, immediately before attaching statuses. + if (run.event === 'pull_request') { + const {data: pr} = await github.rest.pulls.get({...context.repo, pull_number: number}); + if (pr.state !== 'open' || !matchesSource(pr) || pr.base.ref !== branch || + pr.base.repo.full_name !== `${context.repo.owner}/${context.repo.repo}`) return; + } else { + // A removed/rebuilt queue entry must not receive a passing result. + let cursor = null, member = false; + do { + const response = await github.graphql(`query($owner:String!,$name:String!,$branch:String!,$cursor:String) { + repository(owner:$owner,name:$name) { mergeQueue(branch:$branch) { + entries(first:100,after:$cursor) { + nodes { headCommit { oid } } + pageInfo { hasNextPage endCursor } + } + } } + }`, {owner: context.repo.owner, name: context.repo.repo, branch, cursor}); + const entries = response.repository.mergeQueue?.entries; + if (!entries) break; + member = entries.nodes.some(entry => entry.headCommit?.oid === head); + if (member || !entries.pageInfo.hasNextPage) break; + const next = entries.pageInfo.endCursor; + if (!next || next === cursor) throw new Error('Invalid queue pagination'); + cursor = next; + } while (true); + const {data: target} = await github.rest.git.getRef({...context.repo, ref: `heads/${branch}`}); + fresh = fresh && base === target.object.sha && (branch === 'next' || policy === target.object.sha); + fresh = fresh && member; + } + if (branch === 'next' && run.event === 'pull_request') { + const {data: target} = await github.rest.git.getRef({...context.repo, ref: 'heads/next'}); + fresh = fresh && base === target.object.sha; + } + // Bootstrap may adopt only the still-current authenticated main tree. + // This value comes from our selector, never from candidate artifacts. + if (process.env.MAIN_SNAPSHOT) { + const {data: currentMain} = await github.rest.git.getRef({...context.repo, ref: 'heads/main'}); + fresh = fresh && branch === 'next' && process.env.MAIN_SNAPSHOT === policy && + currentMain.object.sha === policy; + } + // Bootstrap authorization was evaluated against authenticated main, + // carried in SELECTED_POLICY_SHA. A newer main requires reevaluation. + if (branch === 'next' && bootstrap === 'true') { + const {data: main} = await github.rest.git.getRef({...context.repo, ref: 'heads/main'}); + fresh = fresh && policy === main.object.sha; + } for (const [name, result] of [ [names[0], process.env.ISOLATION_RESULT], [names[1], process.env.BUDGET_RESULT], @@ -704,34 +753,47 @@ jobs: with: script: | const marker = ''; - const run = context.payload.workflow_run; + const event = context.payload.workflow_run; + const {data: run} = await github.rest.actions.getWorkflowRun({...context.repo, run_id: event.id}); const {data: workflow} = await github.rest.actions.getWorkflow({ ...context.repo, workflow_id: 'castiron-custom-code.yml', }); if (!Number.isSafeInteger(run.workflow_id) || run.workflow_id !== workflow.id || workflow.path !== '.github/workflows/castiron-custom-code.yml') return; - if (run.event !== 'pull_request' || run.path.split('@', 1)[0] !== '.github/workflows/castiron-custom-code.yml') return; + if (run.id !== event.id || run.event !== 'pull_request' || run.event !== event.event || + run.head_sha !== event.head_sha || run.run_attempt !== event.run_attempt || run.status !== 'completed' || + run.path.split('@', 1)[0] !== '.github/workflows/castiron-custom-code.yml' || + run.repository.full_name !== `${context.repo.owner}/${context.repo.repo}`) return; + const matchesSource = pr => Boolean(run.head_repository?.id && run.head_repository?.full_name && run.head_branch && + pr.head?.sha === run.head_sha && pr.head?.ref === run.head_branch && + pr.head?.repo?.id === run.head_repository.id && pr.head?.repo?.full_name === run.head_repository.full_name); + if (!run.head_repository?.id || !run.head_repository.full_name || !run.head_branch) return; + const explicitAssociation = Boolean(run.pull_requests?.length); let pulls = run.pull_requests?.length ? run.pull_requests : await github.paginate(github.rest.repos.listPullRequestsAssociatedWithCommit, {...context.repo, commit_sha: run.head_sha}); - if (!pulls.length) pulls = await github.paginate(github.rest.pulls.list, { - ...context.repo, state: 'open', head: `${run.head_repository.owner.login}:${run.head_branch}`, - }); const current = []; - for (const pull of pulls) { - const {data: pr} = await github.rest.pulls.get({...context.repo, pull_number: pull.number}); - if (pr.state === 'open' && pr.head.sha === run.head_sha && - ['main', 'next'].includes(pr.base.ref) && pr.base.repo.full_name === `${context.repo.owner}/${context.repo.repo}`) current.push(pr); + for (let attempt = 0; attempt < 2; attempt++) { + for (const number of new Set(pulls.map(pull => pull.number))) { + const {data: pr} = await github.rest.pulls.get({...context.repo, pull_number: number}); + if (pr.state === 'open' && matchesSource(pr) && ['main', 'next'].includes(pr.base.ref) && + pr.base.repo.full_name === `${context.repo.owner}/${context.repo.repo}`) current.push({number, branch: pr.base.ref}); + } + if (current.length || explicitAssociation || attempt) break; + pulls = await github.paginate(github.rest.pulls.list, { + ...context.repo, state: 'open', head: `${run.head_repository.owner.login}:${run.head_branch}`, + }); } - // Match trusted compute's supported-target and main-preference selection. - const main = current.filter(pr => pr.base.ref === 'main'); + const main = current.filter(pr => pr.branch === 'main'); const selected = main.length ? main : current; if (selected.length !== 1) return; - for (const pull of selected) { - const comments = await github.paginate(github.rest.issues.listComments, {...context.repo, issue_number: pull.number}); - const previous = comments.find(c => c.user?.type === 'Bot' && c.user?.login === 'github-actions[bot]' && c.body?.startsWith(marker)); - const prior = previous?.body?.match(//); - if (prior && (Number(prior[1]) > run.id || (Number(prior[1]) === run.id && Number(prior[2]) > run.run_attempt))) continue; - const url = `${context.serverUrl}/${context.repo.owner}/${context.repo.repo}/actions/runs/${run.id}`; - const body = `${marker}\n\n## Castiron custom code\n\n⚠️ Report unavailable for \`${run.head_sha.slice(0, 12)}\`.\n\nThe report setup or validation failed. [Inspect the workflow run](${url}).\n\n`; - if (previous) await github.rest.issues.updateComment({...context.repo, comment_id: previous.id, body}); - else await github.rest.issues.createComment({...context.repo, issue_number: pull.number, body}); - } + const {number, branch} = selected[0]; + const comments = await github.paginate(github.rest.issues.listComments, {...context.repo, issue_number: number}); + const previous = comments.find(c => c.user?.type === 'Bot' && c.user?.login === 'github-actions[bot]' && c.body?.startsWith(marker)); + const prior = previous?.body?.match(//); + if (prior && (Number(prior[1]) > run.id || (Number(prior[1]) === run.id && Number(prior[2]) > run.run_attempt))) return; + const {data: pr} = await github.rest.pulls.get({...context.repo, pull_number: number}); + if (pr.state !== 'open' || !matchesSource(pr) || pr.base.ref !== branch || + pr.base.repo.full_name !== `${context.repo.owner}/${context.repo.repo}`) return; + const url = `${context.serverUrl}/${context.repo.owner}/${context.repo.repo}/actions/runs/${run.id}`; + const body = `${marker}\n\n## Castiron custom code\n\n⚠️ Report unavailable for \`${run.head_sha.slice(0, 12)}\`.\n\nThe report setup or validation failed. [Inspect the workflow run](${url}).\n\n`; + if (previous) await github.rest.issues.updateComment({...context.repo, comment_id: previous.id, body}); + else await github.rest.issues.createComment({...context.repo, issue_number: number, body}); diff --git a/.github/workflows/castiron-custom-code.yml b/.github/workflows/castiron-custom-code.yml index e553b6b013..fc26e2e8f0 100644 --- a/.github/workflows/castiron-custom-code.yml +++ b/.github/workflows/castiron-custom-code.yml @@ -14,11 +14,11 @@ permissions: contents: read concurrency: - group: castiron-custom-code-${{ github.event.pull_request.number || github.event.merge_group.head_ref }} + group: castiron-custom-code-${{ github.event_name }}-${{ github.event.pull_request.number || github.event.merge_group.head_ref }} cancel-in-progress: false env: - REPORTER_SHA256: a52a1aabf4047f4c20e96d1d788c25a4aa78c1987b4fa73a13d22c92ef213a3f + REPORTER_SHA256: c243015818640b1e7068605c5352fcb770d406202ee4e75316fe0ca85d06a1cc jobs: queue-signal: diff --git a/docs/castiron-generated-code-transition.md b/docs/castiron-generated-code-transition.md index 002b4c9441..f623e02c29 100644 --- a/docs/castiron-generated-code-transition.md +++ b/docs/castiron-generated-code-transition.md @@ -6,7 +6,25 @@ Eliminate handwritten patches in generated-owned files while preserving every ex No removals or renames of public symbols, new required arguments, narrower input/return types, dependency or Python-floor changes, altered defaults, new payload limits, changed warnings, or changed sync/async behavior are authorized by this transition. Deprecated methods and compatibility import paths remain supported. -## Current public baseline and measured final candidate — 2026-10-22 +## Verified public baseline — 2026-10-26 + +The strict public report at `904ed3f6ba0f2351af67c9456e9f020401a8a7f8` measured **0 generated files with custom edits / 0 custom patch lines**. These counts carry forward to public main `06dbb5d019d7b6870b5898bf5df579ca60b61d28`: a full-tree comparison found only a change to the SDK-owned `create-releases.yml` workflow, outside generated ownership. All other paths, including all **2,025 generated manifest entries**, retain identical bytes and Git modes. The report verified the generated snapshot hash; its two omitted generated workflows were separately authenticated against the manifest and private checkpoint. + +The original `cfa0a79b6395bb0561b28879122a9f90a87cdada` audit measured **41 mixed files / 7,409 custom patch lines**, comprising 4,890 additions and 2,519 deletions. Patch lines measure differences from generated output, including removed lines; they are not the total size of handwritten runtime modules or compiler templates. + +## Current incremental plan + +The mixed-file count reached zero before this follow-up. Remaining work simplifies how the compiler expresses the preserved SDK interfaces: ordinary templates consume native compiler facts, while runtime algorithms remain in wholly handwritten modules. Do not introduce a new facade framework or developer-facing breaking changes. + +The first slice covers Chat parsing/streaming and chunked uploads, with separate compiler reviews followed by private SDK adoption and public promotion where there is an actual SDK change. Subsequent small slices cover Files/Artifacts, Sessions, Threads/Runs, Responses, and client/package wiring, followed by removal of unused declaration machinery after a compatibility audit. + +The remaining inventory contains nine declaration blocks totaling **16,555 authored YAML lines**, plus **75 residual binding lines** tracked separately. These measure compiler configuration still to simplify, not custom patches in SDK files. + +For each slice, compare generated output against the existing implementation using the same API/configuration inputs. Preserve method signatures, annotations, defaults, imports, sync/async behavior, and runtime algorithms. Review any other generated drift separately, run the relevant compatibility and repository checks, keep the verified mixed-file and patch-line counts at zero, and satisfy ordinary review and merge gates. + +The dated audits and original work tracks below are historical records. Their pending-delivery statements and proposed declaration machinery describe the earlier approach; this incremental plan supersedes them. + +## Historical public baseline and candidate — 2026-10-22 The released baseline `cfa0a79b6395bb0561b28879122a9f90a87cdada` (3.37.0) contains **41 mixed files / 7,409 custom patch lines** (4,890 additions and 2,519 deletions), spanning **39,960 physical lines**. Its public generated checkpoint is `992b9cbba312b4bf10128b9f1d36112b48340363`. diff --git a/scripts/castiron/CUSTOM_CODE.md b/scripts/castiron/CUSTOM_CODE.md index 50b102fd27..87cee14efb 100644 --- a/scripts/castiron/CUSTOM_CODE.md +++ b/scripts/castiron/CUSTOM_CODE.md @@ -244,6 +244,16 @@ for its own initialization. A malformed base pin cannot enter this path; other changes without a base pin fail closed. Ordinary pin updates execute under the previous approved version and cannot include SDK/tooling changes. +An existing unpinned `next` may instead adopt the complete authenticated current-main +snapshot when next is an ancestor of main and main already uses strict ownership +with zero custom lines. The canonical `sdk prepare-next-baseline` command preserves +all main bytes, modes and existing public checkpoints, adding only the exact +same-main policy pin. This exception cannot include additional edits, a different +pin, a relaxed policy or multiple queue constituents. Ordinary checkpoint, manifest, +zero-budget and repository review checks remain mandatory. Main must still match +at final publication; if it moves, prepare and qualify a fresh candidate. This is +adoption of already-public output, not regeneration or a budget increase. + Keep required status activation separate until hosted tests have exercised main and next PRs, forks, retargeting, policy changes, and both merge queues. Require the target-specific next contexts after observing their genuine producers; diff --git a/scripts/castiron/custom_code_budget.py b/scripts/castiron/custom_code_budget.py index e1dd34e072..01d21ee8cb 100755 --- a/scripts/castiron/custom_code_budget.py +++ b/scripts/castiron/custom_code_budget.py @@ -33,6 +33,7 @@ read_policy = policy.read_policy changed_paths = policy.changed_paths queued_entries = policy.queued_entries +is_main_snapshot_bootstrap = policy.is_main_snapshot_bootstrap POLICY = ".castiron-ratchet.json" NEXT_POLICY_VERSION = 1 @@ -341,6 +342,8 @@ def evaluate( measurement: tuple[dict[str, Any], bytes] | None = None, policy_sha: str | None = None, target_branch: str = "main", + main_snapshot: str | None = None, + repository: str | None = None, ) -> tuple[dict[str, Any], bytes]: """In a queue, isolate each PR but budget the complete merged candidate.""" base, head = report.require_sha(base), report.require_sha(head) @@ -360,12 +363,36 @@ def evaluate( read_budget(repo, head) if pull_heads is not None and not pull_heads: raise ValueError("merge group has no verified constituent PRs") + if main_snapshot is not None and ( + target_branch != "next" + or main_snapshot != policy + or repository is None + or (pull_heads is not None and len(pull_heads) != 1) + or not is_main_snapshot_bootstrap( + repo, + repository, + base, + head, + policy, + constituent=pull_heads[0] + if pull_heads is not None and len(pull_heads) == 1 + else None, + ) + or any( + not is_main_snapshot_bootstrap(repo, repository, base, candidate, policy) + for candidate in (pull_heads if pull_heads is not None else [head]) + ) + ): + raise ValueError("invalid authenticated main snapshot bootstrap") for pr_head in pull_heads if pull_heads is not None else [head]: - proposed = check_isolation(repo, base, pr_head) + proposed = 0 if main_snapshot is not None else check_isolation(repo, base, pr_head) if proposed is not None: proposed_limits.append(proposed) checks["isolation"] = outcome( - "success", "Budget changes are isolated; human review is required for increases." + "success", + "Exact authenticated strict-zero main snapshot adoption." + if main_snapshot is not None + else "Budget changes are isolated; human review is required for increases.", ) except (report.ReportError, ValueError, UnicodeError) as exc: checks["isolation"] = outcome("failure", str(exc)) @@ -638,6 +665,35 @@ def github_evaluate( pull_heads.append(candidate) if not pull_heads: raise ValueError("cannot verify merge-group PR membership; retry the current group") + main_snapshot = selection.get("main_snapshot") or None + if main_snapshot is not None: + if ( + branch != "next" + or main_snapshot != main + or report.api("GET", f"{root}/git/ref/heads/main")["object"]["sha"] != main + ): + raise ValueError("authenticated main snapshot changed; retry bootstrap") + # This evaluator has its own bare object store, including when reusing + # the PR report. SHA fetches do not create the selector's named main ref. + report.git( + repo, + "fetch", + "--quiet", + "--no-tags", + "origin", + "+refs/heads/main:refs/remotes/origin/main", + ) + if report.git(repo, "rev-parse", "refs/remotes/origin/main").decode().strip() != main: + raise ValueError("authenticated main changed during snapshot fetch; retry") + if not is_main_snapshot_bootstrap( + repo, + repository, + base, + head, + main, + constituent=pull_heads[0] if pull_heads is not None and len(pull_heads) == 1 else None, + ): + raise ValueError("authenticated main snapshot changed; retry bootstrap") if branch == "next": assert selection is not None base_pin = read_policy(repo, base).get("next_policy") @@ -646,7 +702,11 @@ def github_evaluate( if ( selection.get("bootstrap") is not True or "next_policy" not in read_policy(repo, head) - or (pull_heads is None and changed_paths(repo, base, head) != {POLICY.encode()}) + or ( + main_snapshot is None + and pull_heads is None + and changed_paths(repo, base, head) != {POLICY.encode()} + ) ): raise ValueError("unreviewed next policy initialization") elif selection.get("bootstrap") is not False or base_pin != { @@ -664,16 +724,150 @@ def github_evaluate( measurement=measurement, policy_sha=main, target_branch=branch, + main_snapshot=main_snapshot, + repository=repository, ) result["target_branch"] = branch return result, patch +def prepare_main_snapshot( + repo: Path, repository: str, base: str, authenticated_main: str, *, fetch: bool = False +) -> str: + """Create and verify an adoption commit without altering a ref, index or metadata.""" + base, authenticated_main = map(report.require_sha, (base, authenticated_main)) + strict = {"schema_version": 2, "max_custom_patch_lines": 0, "generated_files": "strict"} + if read_policy(repo, authenticated_main) != strict or "next_policy" in read_policy(repo, base): + raise ValueError("main snapshot preparation requires strict-zero main and unpinned next") + if policy.merge_base(repo, base, authenticated_main) != base: + raise ValueError("protected next must be an ancestor of authenticated main") + value = {**strict, "next_policy": {"repository": repository, "main_sha": authenticated_main}} + blob = report.git( + repo, + "hash-object", + "-w", + "--stdin", + input_bytes=(json.dumps(value, indent=2) + "\n").encode(), + ).strip() + entries = report.git(repo, "ls-tree", "-z", authenticated_main).split(b"\0") + # Replace only the root policy blob, preserving every other entry and mode. + replacement = b"100644 blob " + blob + b"\t" + POLICY.encode() + tree = ( + report.git( + repo, + "mktree", + "-z", + input_bytes=b"\0".join( + replacement if entry.partition(b"\t")[2] == POLICY.encode() else entry + for entry in entries + ), + ) + .decode() + .strip() + ) + head = ( + report.git( + repo, + "commit-tree", + tree, + "-p", + base, + "-p", + authenticated_main, + "-m", + "Adopt verified strict-zero main baseline on next", + ) + .decode() + .strip() + ) + if not is_main_snapshot_bootstrap(repo, repository, base, head, authenticated_main): + raise ValueError("prepared snapshot does not match authenticated main") + result, _ = evaluate( + repo, + base, + head, + public=True, + fetch=fetch, + target_branch="next", + policy_sha=authenticated_main, + main_snapshot=authenticated_main, + repository=repository, + ) + if not all(check["state"] == "success" for check in result["checks"].values()): + raise ValueError("main snapshot verification failed: " + json.dumps(result["checks"])) + return cast(str, head) + + +def read_public_api(path: str) -> Any: + # Reuse the local gh login without reading or printing its credential. + output = subprocess.run( + ["gh", "api", "--hostname", "github.com", "--method", "GET", path], + capture_output=True, + check=True, + timeout=60, + ) + return json.loads(output.stdout) + + +def prepare_next_baseline(repo: Path, repository: str, branch: str) -> dict[str, str]: + """Authenticate public refs and create one new local review branch; never push.""" + if not report.REPOSITORY.fullmatch(repository) or not branch.startswith("codex/"): + raise ValueError("expected a repository and a new codex/ review branch") + report.git(repo, "check-ref-format", "--branch", branch) + if ( + report.git(repo, "remote", "get-url", "origin").decode().strip() + != f"https://github.com/{repository}.git" + ): + raise ValueError("preparation requires the public repository as origin") + root = f"repos/{repository}" + metadata = read_public_api(root) + if ( + metadata["private"] + or metadata["default_branch"] != "main" + or metadata["full_name"] != repository + ): + raise ValueError("preparation requires a public repository with default main") + + def ref(name: str) -> str: + return cast( + str, + report.require_sha(read_public_api(f"{root}/git/ref/heads/{name}")["object"]["sha"]), + ) + + main, base = ref("main"), ref("next") + report.git( + repo, + "fetch", + "--quiet", + "--no-tags", + "origin", + "+refs/heads/main:refs/remotes/origin/main", + "+refs/heads/next:refs/remotes/origin/next", + ) + for name, expected in (("main", main), ("next", base)): + if ( + report.git(repo, "rev-parse", f"refs/remotes/origin/{name}").decode().strip() + != expected + ): + raise ValueError("public refs changed during preparation; retry") + head = prepare_main_snapshot(repo, repository, base, main, fetch=True) + if ref("main") != main or ref("next") != base: + raise ValueError("public refs changed during verification; retry") + # Compare-and-create only. Existing review branches and working trees are preserved. + report.git(repo, "update-ref", f"refs/heads/{branch}", head, "0" * 40) + return {"head_sha": head, "base_sha": base, "main_sha": main, "branch": branch} + + def main() -> int: if sys.argv[1:2] == ["select-policy"]: return cast(int, policy.main(sys.argv[1:])) parser = argparse.ArgumentParser(description=__doc__) commands = parser.add_subparsers(dest="command", required=True) + commands.add_parser("review-rules") + prepare = commands.add_parser("prepare-next-baseline") + prepare.add_argument("--repo", type=Path, required=True) + prepare.add_argument("--repository", required=True) + prepare.add_argument("--branch", required=True) generated = commands.add_parser("verify-generated") generated.add_argument("--repo", type=Path, required=True) generated.add_argument("--policy-ref", required=True) @@ -697,6 +891,17 @@ def main() -> int: command.add_argument("--repo", type=Path, required=True) command.add_argument("--out", type=Path, required=True) args = parser.parse_args() + if args.command == "review-rules": + print(json.dumps(policy.HUMAN_REVIEW_RULES, separators=(",", ":"))) + return 0 + if args.command == "prepare-next-baseline": + try: + preparation = prepare_next_baseline(args.repo, args.repository, args.branch) + except (ValueError, OSError, report.ReportError, subprocess.SubprocessError) as exc: + print(str(exc), file=sys.stderr) + return 1 + print(json.dumps(preparation, sort_keys=True)) + return 0 if args.command == "verify-generated": # Local pipeline integration pins policy and checkpoint before merging. # Legacy SDKs without a policy retain their existing behavior. diff --git a/scripts/castiron/custom_code_policy.py b/scripts/castiron/custom_code_policy.py index 6f26b05ec8..35538555ea 100755 --- a/scripts/castiron/custom_code_policy.py +++ b/scripts/castiron/custom_code_policy.py @@ -110,7 +110,7 @@ def next_policy_sha(repo: Path, repository: str, base: str, authenticated_main: raise ValueError("trusted next base has no reviewed policy pin") if pin["repository"] != repository: raise ValueError("next policy pin belongs to another repository") - policy = report.require_sha(pin["main_sha"]) + policy = cast(str, report.require_sha(pin["main_sha"])) # A reachable side-branch commit is not an adopted main policy revision. adopted = report.git(repo, "rev-list", "--first-parent", authenticated_main).splitlines() if policy.encode() not in adopted: @@ -132,7 +132,7 @@ def next_policy_sha(repo: Path, repository: str, base: str, authenticated_main: ) if not capable: raise ValueError("pinned checker does not support reviewed next policies") - return cast(str, policy) + return policy def supports_effective_pin_review(repo: Path, revision: str) -> bool: @@ -197,6 +197,19 @@ def require_human_policy_review(repository: str, head: str, number: int | None = if number is not None else report.api("GET", f"{root}/commits/{head}/pulls?per_page=100") ) + if not pulls: + # Fork runs can have no commit association. Discover candidates from + # the authenticated base repository, then apply the same live checks + # below. Never accept an approval based only on search metadata. + page = 1 + while True: + candidates = report.api( + "GET", f"{root}/pulls?state=open&base=next&per_page=100&page={page}" + ) + pulls.extend(item for item in candidates if item["head"]["sha"] == head) + if len(candidates) < 100: + break + page += 1 current: list[dict[str, Any]] = [] for item in pulls: pull = report.api("GET", f"{root}/pulls/{item['number']}") @@ -280,6 +293,67 @@ def requires_human_policy_review( return budget_increased or ratchet_activated or pin_requires_human +def is_main_snapshot_bootstrap( + repo: Path, + repository: str, + base: str, + head: str, + authenticated_main: str, + *, + constituent: str | None = None, +) -> bool: + """Recognize only an exact, strict-zero authenticated main snapshot adoption. + + This does not verify generated ownership: the ordinary budget evaluator must + still authenticate the unchanged main checkpoints and their complete manifest. + No candidate file is executed and no generation metadata is rewritten. + """ + base, head, authenticated_main = map(report.require_sha, (base, head, authenticated_main)) + if "next_policy" in read_policy(repo, base): + return False + strict = {"schema_version": 2, "max_custom_patch_lines": 0, "generated_files": "strict"} + if read_policy(repo, authenticated_main) != strict: + return False + expected_policy = { + **strict, + "next_policy": {"repository": repository, "main_sha": authenticated_main}, + } + if read_policy(repo, head) != expected_policy: + return False + # Also validates the configured repository, fetched main and protocol marker. + if next_policy_sha(repo, repository, head, authenticated_main) != authenticated_main: + return False + if merge_base(repo, base, authenticated_main) != base or merge_base(repo, base, head) != base: + return False + paths = set( + report.git( + repo, + "diff", + "--name-only", + "-z", + "--no-renames", + "--no-ext-diff", + "--no-textconv", + authenticated_main, + head, + "--", + ).split(b"\0") + ) - {b""} + if paths != {POLICY.encode()}: + return False + parents = report.git(repo, "rev-list", "--parents", "-n", "1", head).decode().split()[1:] + expected = list(dict.fromkeys((base, authenticated_main))) + if constituent is not None and head != constituent: + if not is_main_snapshot_bootstrap(repo, repository, base, constituent, authenticated_main): + return False + expected = [base, constituent] + if parents != expected: + return False + # The complete tree comparison binds stats/inventory bytes and file modes. + # Require an existing public snapshot; this is not legacy metadata recovery. + return bool(report.read_stats(repo, authenticated_main).get("public_codegen_sha")) + + def select_policy( repo: Path, repository: str, @@ -308,9 +382,30 @@ def select_policy( base_policy = read_policy(repo, base) head_policy = read_policy(repo, head) bootstrap = "next_policy" not in base_policy + snapshot_bootstrap = bootstrap and is_main_snapshot_bootstrap( + repo, + repository, + base, + head, + authenticated_main, + constituent=pull_heads[0] if pull_heads is not None and len(pull_heads) == 1 else None, + ) + if ( + snapshot_bootstrap + and pull_heads is not None + and ( + len(pull_heads) != 1 + or not is_main_snapshot_bootstrap( + repo, repository, base, pull_heads[0], authenticated_main + ) + ) + ): + raise ValueError("main snapshot bootstrap requires one exact verified constituent") if bootstrap: if "next_policy" not in head_policy or ( - pull_heads is None and changed_paths(repo, base, head) != {POLICY.encode()} + not snapshot_bootstrap + and pull_heads is None + and changed_paths(repo, base, head) != {POLICY.encode()} ): raise ValueError("missing next pin allows only reviewed policy-only initialization") policy = authenticated_main @@ -320,11 +415,14 @@ def select_policy( head_base_policy = read_policy(repo, merge_base(repo, base, head)) if head_policy.get("next_policy") != head_base_policy.get("next_policy"): if "next_policy" not in head_policy or ( - pull_heads is None and changed_paths(repo, base, head) != {POLICY.encode()} + not snapshot_bootstrap + and pull_heads is None + and changed_paths(repo, base, head) != {POLICY.encode()} ): raise ValueError("next pin changes must be policy-only and cannot remove the pin") next_policy_sha(repo, repository, head, authenticated_main) policy_reviews: list[dict[str, Any]] = [] + bootstrap_validated = False candidates = pull_heads if pull_heads is not None else [head] if not candidates: raise ValueError("next evaluation has no verified constituent PR") @@ -342,15 +440,23 @@ def select_policy( ratchet_activated = ( candidate_policy.get("ratchet") == "go" and candidate_base_policy.get("ratchet") != "go" ) - if (pin_changed or budget_increased or ratchet_activated) and changed_paths( - repo, base, candidate - ) != {POLICY.encode()}: + if ( + not snapshot_bootstrap + and (pin_changed or budget_increased or ratchet_activated) + and changed_paths(repo, base, candidate) != {POLICY.encode()} + ): raise ValueError("each next policy activation or increase must be policy-only") if requires_human_policy_review(repo, repository, base, candidate, authenticated_main): number = require_human_policy_review( repository, candidate, pull_numbers[candidate] if pull_numbers is not None else None ) policy_reviews.append({"number": number, "head_sha": candidate}) + # Keep activation tied to its verified policy-only constituent, even + # when current main's effective-policy rules require no new approval. + if pin_changed and candidate_policy.get("next_policy") == head_policy.get("next_policy"): + bootstrap_validated = True + if bootstrap and not bootstrap_validated: + raise ValueError("bootstrap requires a verified policy-only constituent") return { "schema_version": SELECTION_RECEIPT_VERSION, "target_branch": branch, @@ -358,6 +464,7 @@ def select_policy( "head_sha": head, "policy_sha": policy, "bootstrap": bootstrap, + "main_snapshot": authenticated_main if snapshot_bootstrap else "", "policy_reviews": policy_reviews, "human_review_rules": HUMAN_REVIEW_RULES, } diff --git a/scripts/castiron/custom_code_report.py b/scripts/castiron/custom_code_report.py index 88c897bbf1..d3fa56c36a 100755 --- a/scripts/castiron/custom_code_report.py +++ b/scripts/castiron/custom_code_report.py @@ -22,6 +22,8 @@ import struct import subprocess import sys +import tempfile +from contextlib import ExitStack from dataclasses import dataclass from pathlib import Path from typing import Any, cast @@ -81,13 +83,21 @@ def git(repo: Path, *args: str, input_bytes: bytes | None = None) -> bytes: ): env.pop(name, None) env["GIT_NO_REPLACE_OBJECTS"] = "1" - result = subprocess.run( - ["git", "--no-replace-objects", "-C", str(repo), *args], - input=input_bytes, - capture_output=True, - env=env, - check=False, - ) + with ExitStack() as resources: + stdin = None + if input_bytes is not None: + # Batched object reads can fill both pipes on macOS. Feed requests + # through a private, automatically removed file while draining output. + stdin = resources.enter_context(tempfile.TemporaryFile()) + stdin.write(input_bytes) + stdin.seek(0) + result = subprocess.run( + ["git", "--no-replace-objects", "-C", str(repo), *args], + stdin=stdin, + capture_output=True, + env=env, + check=False, + ) if result.returncode: raise ReportError(f"git {args[0]} failed: {result.stderr.decode(errors='replace').strip()}") return result.stdout @@ -1150,39 +1160,74 @@ def api(method: str, path: str, payload: dict[str, Any] | None = None) -> Any: return json.loads(result.stdout) -def associated_pulls(repository: str, run: dict[str, Any]) -> list[dict[str, Any]]: +def associated_pulls( + repository: str, run: dict[str, Any], *, branch_only: bool = False +) -> list[dict[str, Any]]: root = f"repos/{repository}" - associated = run["pull_requests"] or api( - "GET", f"{root}/commits/{require_sha(run['head_sha'])}/pulls?per_page=100" + associated: list[dict[str, Any]] = ( + [] + if branch_only + else run["pull_requests"] + or api("GET", f"{root}/commits/{require_sha(run['head_sha'])}/pulls?per_page=100") ) if associated: - return cast(list[dict[str, Any]], associated) + return associated # Fork runs can be absent from both association endpoints. Discover candidates # by branch; callers still verify the current PR head and target through GitHub. - head = f"{run['head_repository']['owner']['login']}:{run['head_branch']}" + source: dict[str, Any] = run.get("head_repository") or {} + if not ( + source.get("id") + and source.get("full_name") + and source.get("owner", {}).get("login") + and run.get("head_branch") + ): + return [] + head = f"{source['owner']['login']}:{run['head_branch']}" for page in range(1, 101): query = urlencode({"state": "open", "head": head, "per_page": 100, "page": page}) pulls = api("GET", f"{root}/pulls?{query}") associated.extend(pulls) if len(pulls) < 100: - return cast(list[dict[str, Any]], associated) + return associated raise ReportError("too many pull requests for source branch") +def matches_source(pull: dict[str, Any], run: dict[str, Any]) -> bool: + """A shared commit is discovery metadata, not authority over another PR.""" + source: dict[str, Any] = run.get("head_repository") or {} + head: dict[str, Any] = pull.get("head") or {} + repository: dict[str, Any] = head.get("repo") or {} + return bool( + source.get("id") + and source.get("full_name") + and run.get("head_branch") + and repository.get("id") == source["id"] + and repository.get("full_name") == source["full_name"] + and head.get("ref") == run["head_branch"] + and head.get("sha") == run["head_sha"] + ) + + def current_pull(repository: str, run: dict[str, Any]) -> tuple[int, str] | None: """Resolve one live supported target; association metadata is discovery only.""" current: list[tuple[int, str]] = [] - for number in sorted({int(pr["number"]) for pr in associated_pulls(repository, run)}): - if number <= 0: - raise ReportError("invalid associated pull request") - pull = api("GET", f"repos/{repository}/pulls/{number}") - if ( - pull["state"] == "open" - and pull["head"]["sha"] == run["head_sha"] - and pull["base"]["repo"]["full_name"] == repository - and pull["base"]["ref"] in TARGET_BRANCHES - ): - current.append((number, pull["base"]["ref"])) + pulls = associated_pulls(repository, run) + for attempt in range(2): + for number in sorted({int(pr["number"]) for pr in pulls}): + if number <= 0: + raise ReportError("invalid associated pull request") + pull = api("GET", f"repos/{repository}/pulls/{number}") + if ( + pull["state"] == "open" + and matches_source(pull, run) + and pull["base"]["repo"]["full_name"] == repository + and pull["base"]["ref"] in TARGET_BRANCHES + ): + current.append((number, pull["base"]["ref"])) + if current or run["pull_requests"] or attempt: + break + # Nonempty commit associations can contain only unrelated PRs. + pulls = associated_pulls(repository, run, branch_only=True) # Fallback discovery can find both targets for one head. Main's required # checks take precedence over next's advisory report; keep same-target # ambiguity fail-closed. Explicit run associations still limit discovery. @@ -1221,11 +1266,10 @@ def publish_comment( or run["head_sha"] != report["head_sha"] ): raise ReportError("workflow run does not match report PR/head") - associated = associated_pulls(repository, run) - if not any(pr["number"] == number for pr in associated): - raise ReportError("workflow run does not match report PR/head") - if run["run_attempt"] != run_attempt: + if run["run_attempt"] != run_attempt or not matches_source(pull, run): return "Skipped stale report" + if current_pull(repository, run) != (number, pull["base"]["ref"]): + raise ReportError("workflow run does not match report PR/head") artifact_run_id = artifact_run_id or run_id artifact_run_attempt = artifact_run_attempt or run_attempt body = render_report( @@ -1261,6 +1305,7 @@ def publish_comment( or pull["base"]["ref"] != report.get("target_branch", "main") or pull["base"]["ref"] not in TARGET_BRANCHES or pull["base"]["repo"]["full_name"] != repository + or not matches_source(pull, run) ): return "Skipped stale report" if found is not None: diff --git a/scripts/castiron/custom_code_test_support.py b/scripts/castiron/custom_code_test_support.py index 9db89fd564..add136e49d 100644 --- a/scripts/castiron/custom_code_test_support.py +++ b/scripts/castiron/custom_code_test_support.py @@ -27,7 +27,10 @@ def setUp(self) -> None: # pyright: ignore[reportImplicitOverride] def git(self, *args: str) -> str: return subprocess.run( - ["git", "-C", str(self.repo), *args], check=True, capture_output=True, text=True + ["git", "-C", str(self.repo), *args], + check=True, + capture_output=True, + text=True, ).stdout.strip() def write(self, path: str, body: str) -> None: @@ -66,7 +69,11 @@ def source_run(head: str, event: str = "pull_request") -> dict[str, Any]: "head_sha": head, "head_branch": "gh-readonly-queue/main/pr-7-example" if event == "merge_group" else "sdk", "repository": {"full_name": "openai/example"}, - "head_repository": {"owner": {"login": "contributor"}}, + "head_repository": { + "id": 10, + "full_name": "contributor/example", + "owner": {"login": "contributor"}, + }, "path": ".github/workflows/castiron-custom-code.yml", "status": "completed", "run_attempt": 1, @@ -75,6 +82,10 @@ def source_run(head: str, event: str = "pull_request") -> dict[str, Any]: } +def source_head(sha: str) -> dict[str, Any]: + return {"sha": sha, "ref": "sdk", "repo": source_run(sha)["head_repository"]} + + WORKFLOW = ( Path(__file__).resolve().parents[2] / ".github/workflows/castiron-custom-code-comment.yml" ) diff --git a/scripts/castiron/fixtures/github_publisher.cjs b/scripts/castiron/fixtures/github_publisher.cjs index c831f52970..58324d2496 100644 --- a/scripts/castiron/fixtures/github_publisher.cjs +++ b/scripts/castiron/fixtures/github_publisher.cjs @@ -11,7 +11,7 @@ const github = { throw Object.assign(new Error('Workflow run lookup failed'), { status: data.run_errors[run_id], }); - return { data: data.runs_by_id?.[run_id] || data.run }; + return { data: data.runs_by_id?.[run_id] || data.run || data.context?.payload?.workflow_run }; }, getWorkflow: async ({ workflow_id }) => ({ data: { @@ -41,7 +41,9 @@ const github = { ? data.merge_sha : ref === 'heads/main' && (data.main_sha_sequence?.length || data.main_sha) ? data.main_sha_sequence?.shift() || data.main_sha - : data.current.base.sha, + : ref === 'heads/next' && (data.target_sha_sequence?.length || data.target_sha) + ? data.target_sha_sequence?.shift() || data.target_sha + : data.current.base.sha, }, }, }), @@ -90,6 +92,7 @@ const github = { if (_query.includes('reviews(first:100')) { const page = data.review_pages?.shift(); if (page && page.expected_cursor !== cursor) throw new Error('Wrong review cursor'); + if (page?.after_read) Object.assign(data, page.after_read); return { repository: { pullRequest: { @@ -122,7 +125,7 @@ const github = { if (method === 'workflow-runs') return data.runs_by_sha?.[params.head_sha] || []; if (method === 'reviews') return data.reviews_by_number?.[params.pull_number] || data.reviews || []; if (method === 'statuses') return data.statuses_by_sha?.[params.ref] || data.previous_statuses || []; - if (method === 'comments') return data.comments || []; + if (method === 'comments') return data.comments_by_number?.[params.issue_number] || data.comments || []; if (method === 'associations') return data.associated_pulls || []; if (method === 'pulls' && params.head === 'contributor:sdk' && params.state === 'open') { return data.fallback_pulls || []; @@ -155,6 +158,8 @@ const exec = { } if (data.policy_error_sequence?.shift() || data.policy_error) throw new Error('Trusted policy evaluation failed'); + const expectedBase = data.expected_policy_bases?.shift() || data.expected_policy_base; + if (expectedBase && args[8] !== expectedBase) throw new Error('Evaluated stale target policy'); return { stdout: JSON.stringify({ requires_human: data.requires_human_by_main?.[args[12]] ?? data.requires_human ?? true, diff --git a/scripts/castiron/generated-files.json b/scripts/castiron/generated-files.json index 02c8af38e7..ef047246a3 100644 --- a/scripts/castiron/generated-files.json +++ b/scripts/castiron/generated-files.json @@ -3,11 +3,11 @@ "files": { ".github/workflows/castiron-custom-code-comment.yml": { "mode": "100644", - "sha256": "bdde9eec1ff9c0e842ddc0f69df5611ad4eb0336602489e2dcddfbcf8ad86e37" + "sha256": "b1057dcb3e913bc04e6b04451ce2c64c95c0c588718f7dfddffef3c38f151844" }, ".github/workflows/castiron-custom-code.yml": { "mode": "100644", - "sha256": "2760a19c1e05a2a01d73a679dac9d59ba7cfee2faa2fe426182f740ae1603202" + "sha256": "ae54d4fd4828221782866f5d1e9fb080206d34ac73c63cd8ed2b6b8e83eac387" }, "api.md": { "mode": "100644", @@ -15,7 +15,7 @@ }, "scripts/castiron/CUSTOM_CODE.md": { "mode": "100644", - "sha256": "8464fb28964ae0f7bec38ae7ba8107e97287d4118657a3017a2712e753d4c608" + "sha256": "c2b12719937fa007d65bea7429962f2b674c531a8c284cc6e5e51c2a526ae351" }, "scripts/castiron/README.md": { "mode": "100644", @@ -23,23 +23,23 @@ }, "scripts/castiron/custom_code_budget.py": { "mode": "100755", - "sha256": "3c96300546afb5e57fbfb596d4e1bde90821928e44904c4aeb91e2d7e4e27ff5" + "sha256": "dc7e39700868edfddd7152fbe03ae7347099a5623a46ec2dcc26650ea804ac06" }, "scripts/castiron/custom_code_policy.py": { "mode": "100755", - "sha256": "7dff1ec5cd69deb85e430015d8f379f042eccbedc1786d3c9e4ed8532dfeca9c" + "sha256": "ae04c9cfc445db50d9d3d346470ef08f60afc04f85c02e9dcb0691e4e16982ee" }, "scripts/castiron/custom_code_report.py": { "mode": "100755", - "sha256": "a52a1aabf4047f4c20e96d1d788c25a4aa78c1987b4fa73a13d22c92ef213a3f" + "sha256": "c243015818640b1e7068605c5352fcb770d406202ee4e75316fe0ca85d06a1cc" }, "scripts/castiron/custom_code_test_support.py": { "mode": "100644", - "sha256": "8fc5147352025e2c874ae855ac8f3ac649d531d2c1bd80c532bc4e9ec4fafa3f" + "sha256": "022cd73039e47e4b390ddad54a6e46b0924efa4e9e047e6f0639924752353b2c" }, "scripts/castiron/fixtures/github_publisher.cjs": { "mode": "100644", - "sha256": "162963b9a13c24443ac1a97f852f269fcde5fb011a0871d428f4585cbaac2dab" + "sha256": "c84bd86518a70a5b7bbf877d8c13129debface0c220e5118510b0ef17d9ab707" }, "scripts/castiron/ruff.toml": { "mode": "100644", @@ -47,27 +47,39 @@ }, "scripts/castiron/test_custom_code_budget.py": { "mode": "100644", - "sha256": "feaf35f7f30deecb89d95b31b8a325033180c64650e1d965cbdee486ab11f16d" + "sha256": "7fdb3a9282978e6b1c0d84d73c43bb4adf96ff035663e77bc8e45f0fe082be7d" }, "scripts/castiron/test_custom_code_github.py": { "mode": "100644", - "sha256": "034eecfd80d3236509d2bf32fb6e69f8f13589c170b93f85935adde3d3b58b42" + "sha256": "8e5c46ce2820f5e8232673da6c75f75a6f289617c30203771fca1d981e793eba" }, "scripts/castiron/test_custom_code_policy.py": { "mode": "100644", - "sha256": "29b8dde7d69cad22fe6757356fa18ef189d7fc8f9349f65aad641005540faa26" + "sha256": "9493ac7d56b6291042eca3b77141e7226b845332c96e6e33aaf90a0687f346d5" + }, + "scripts/castiron/test_custom_code_policy_source.py": { + "mode": "100644", + "sha256": "dcb0be5e8157791dc02d1ae1d6b82cc703d4bc7b3c4797bc00bd9d2672ad13a2" }, "scripts/castiron/test_custom_code_publication.py": { "mode": "100644", - "sha256": "0e7628d6906153029fea156273094a3d0b17846abd0894a91ab0833241bfdf31" + "sha256": "7a5a381cad6106a079e0e1eb9415bbb6393e7e7317d020b869ad1338201632de" }, "scripts/castiron/test_custom_code_report.py": { "mode": "100644", - "sha256": "f4d90fd38b17aac59096488391b65ef9da5493d2049d4c038d5926a960257ca7" + "sha256": "e1fbafb5242cac6128757339d78902b12e7658c82e5f157cc2ca7bef80347838" }, "scripts/castiron/test_custom_code_review_revocation.py": { "mode": "100644", - "sha256": "6d971965f6b4da770f4d49b3587dbd136c5be549453ae0d220150fc537b2736a" + "sha256": "c2542298548bd00d522ba448685e998897de82cfcc967f8e1b0297e5fbcec141" + }, + "scripts/castiron/test_custom_code_status_publication.py": { + "mode": "100644", + "sha256": "c3479390737e088ccb3d31926ae2636f763fc6919b3f871d21f066fed6802bfd" + }, + "scripts/castiron/test_custom_code_strict.py": { + "mode": "100644", + "sha256": "3b61e1bde4bfc80cf657746f482fe334a9e0c7517262d3eb23b940ef49a0c374" }, "src/openai/__init__.py": { "mode": "100644", @@ -75,7 +87,7 @@ }, "src/openai/_client.py": { "mode": "100644", - "sha256": "96f6317992c8a8cd8dc66c5e95bb5de6cae7e681e881bc00d12b0e12191d18d2" + "sha256": "5d15ec6b96667bbce3d926f1d7a0eede0d88876773193c6f92e9bf4cd63239d0" }, "src/openai/_data_residency.py": { "mode": "100644", diff --git a/scripts/castiron/test_custom_code_budget.py b/scripts/castiron/test_custom_code_budget.py index b35e8fad44..1290c57161 100644 --- a/scripts/castiron/test_custom_code_budget.py +++ b/scripts/castiron/test_custom_code_budget.py @@ -2,7 +2,6 @@ # Regression tests for the custom-code budget. from __future__ import annotations -import hashlib import json import subprocess import sys @@ -12,7 +11,6 @@ from unittest import mock import custom_code_budget as budget -import custom_code_report as report from custom_code_test_support import GitTestCase @@ -428,492 +426,5 @@ def test_inventory_rejects_missing_ambiguous_and_nonliteral_values(self) -> None budget.checkpoint_github_inventory(source) -class StrictTests(GitTestCase): - workflow = ".github/workflows/castiron-custom-code.yml" - - def setUp(self) -> None: # pyright: ignore[reportImplicitOverride] - super().setUp() - self.write("generated.py", "generated\n") - for path in budget.GENERATED_GITHUB_PATHS: - self.write(path.decode(), "workflow\n") - self.inventory(budget.GENERATED_GITHUB_PATHS) - self.manifest() - self.generated, _ = self.baseline() - self.write( - budget.POLICY, - json.dumps( - {"schema_version": 2, "generated_files": "strict", "max_custom_patch_lines": 0} - ), - ) - self.base = self.commit("enable strict ownership") - - def inventory(self, paths: frozenset[bytes]) -> None: - # Checkpoint code is data. A verifier must never execute this statement. - self.write( - "scripts/castiron/custom_code_budget.py", - "raise AssertionError('checkpoint code must not execute')\n" - f"GENERATED_GITHUB_PATHS = frozenset({set(paths)!r})\n", - ) - - def manifest(self, extra: tuple[str, ...] = ()) -> None: - files = { - p: { - "mode": "100644", - "sha256": hashlib.sha256((self.repo / p).read_bytes()).hexdigest(), - } - for p in ( - "generated.py", - *(path.decode() for path in budget.GENERATED_GITHUB_PATHS), - "scripts/castiron/custom_code_budget.py", - *extra, - ) - } - self.write(budget.MANIFEST.decode(), json.dumps({"schema_version": 1, "files": files})) - - def evaluate(self) -> dict[str, Any]: - return budget.evaluate(self.repo, self.base, self.commit(), public=False)[0] - - def test_handwritten_change_passes(self) -> None: - self.write("handwritten.py", "custom behavior\n") - self.assertEqual(self.evaluate()["checks"]["budget"]["state"], "success") - - def test_strict_main_checks_migration_target_and_candidate(self) -> None: - self.git("checkout", "-q", "-b", "next", self.base) - self.write(budget.POLICY, '{"schema_version":1,"max_custom_patch_lines":1000}') - target = self.commit("next still uses migration policy") - self.write(self.workflow, "manual workflow edit\n") - head = self.commit() - for pull_heads in (None, [head]): - with self.subTest(pull_heads=pull_heads): - result, _ = budget.evaluate( - self.repo, - target, - head, - public=False, - policy_sha=self.base, - pull_heads=pull_heads, - ) - self.assertEqual(result["total"], 0) - self.assertEqual(result["checks"]["isolation"]["state"], "success") - self.assertEqual(result["checks"]["budget"]["state"], "failure") - self.assertIn(self.workflow, result["strict_violations"][0]) - - def test_activation_accepts_stale_pr_without_policy_edits(self) -> None: - strict = (self.repo / budget.POLICY).read_text() - self.git("checkout", "-q", "-b", "migration", f"{self.base}^") - self.write(budget.POLICY, '{"schema_version":1,"max_custom_patch_lines":1000}') - migration = self.commit("migration policy before activation") - self.git("checkout", "-q", "-b", "stale", migration) - self.write("handwritten.py", "custom behavior\n") - stale = self.commit("PR predates strict activation") - self.git("checkout", "-q", "migration") - self.write(budget.POLICY, strict) - activated = self.commit("activate strict policy") - result, _ = budget.evaluate(self.repo, activated, stale, public=False) - self.assertTrue(all(c["state"] == "success" for c in result["checks"].values())) - self.git("merge", "--no-ff", "-m", "queue stale PR", stale) - result, _ = budget.evaluate( - self.repo, - activated, - self.git("rev-parse", "HEAD"), - public=False, - pull_heads=[stale], - ) - self.assertTrue(all(c["state"] == "success" for c in result["checks"].values())) - - def test_stale_policy_edit_cannot_downgrade_activated_base(self) -> None: - strict = (self.repo / budget.POLICY).read_text() - self.git("checkout", "-q", "-b", "migration", f"{self.base}^") - self.write(budget.POLICY, '{"schema_version":1,"max_custom_patch_lines":10}') - migration = self.commit("migration policy before activation") - self.git("checkout", "-q", "-b", "stale", migration) - self.write(budget.POLICY, '{"schema_version":1,"max_custom_patch_lines":1000}') - stale = self.commit("stale policy increase") - self.git("checkout", "-q", "migration") - self.write(budget.POLICY, strict) - activated = self.commit("activate strict policy") - result, _ = budget.evaluate(self.repo, activated, stale, public=False) - self.assertEqual(result["checks"]["isolation"]["state"], "failure") - self.assertIn("cannot be downgraded", result["checks"]["isolation"]["description"]) - - def test_workflow_edit_fails_despite_zero_legacy_patch(self) -> None: - self.write(self.workflow, "edited workflow\n") - result = self.evaluate() - self.assertEqual(result["total"], 0) - self.assertEqual(result["checks"]["budget"]["state"], "failure") - self.assertIn(self.workflow, result["strict_violations"][0]) - - def test_merge_queue_checks_combined_generated_ownership(self) -> None: - self.git("checkout", "-q", "-b", "handwritten", self.base) - self.write("handwritten.py", "custom behavior\n") - handwritten = self.commit() - self.git("checkout", "-q", "-b", "workflow", self.base) - self.write(self.workflow, "manual workflow edit\n") - workflow = self.commit() - self.git("checkout", "-q", "-b", "queue", self.base) - self.git("merge", "--no-ff", "-m", "queue handwritten", handwritten) - passing, _ = budget.evaluate( - self.repo, - self.base, - self.git("rev-parse", "HEAD"), - public=False, - pull_heads=[handwritten], - ) - self.assertTrue(all(c["state"] == "success" for c in passing["checks"].values())) - self.git("merge", "--no-ff", "-m", "queue workflow", workflow) - failing, _ = budget.evaluate( - self.repo, - self.base, - self.git("rev-parse", "HEAD"), - public=False, - pull_heads=[handwritten, workflow], - ) - self.assertEqual(failing["total"], 0) - self.assertEqual(failing["checks"]["isolation"]["state"], "success") - self.assertEqual(failing["checks"]["budget"]["state"], "failure") - self.assertIn(self.workflow, failing["strict_violations"][0]) - - def test_content_deletion_rename_mode_and_symlink_fail(self) -> None: - for mutation in ("edit", "delete", "rename", "mode", "symlink"): - with self.subTest(mutation=mutation): - self.git("reset", "--hard", self.base) - path = self.repo / "generated.py" - if mutation == "edit": - path.write_text("header removed and manually changed\n") - elif mutation == "delete": - path.unlink() - elif mutation == "rename": - path.rename(self.repo / "renamed.py") - elif mutation == "mode": - path.chmod(0o755) - else: - path.unlink() - path.symlink_to("handwritten.py") - self.assertEqual(self.evaluate()["checks"]["budget"]["state"], "failure") - - def test_candidate_cannot_rewrite_its_manifest_or_disable_strict(self) -> None: - self.write(self.workflow, "custom\n") - self.manifest() - self.write(budget.POLICY, json.dumps({"schema_version": 1, "max_custom_patch_lines": 1000})) - result = self.evaluate() - self.assertEqual(result["checks"]["isolation"]["state"], "failure") - self.assertEqual(result["checks"]["budget"]["state"], "failure") - - def test_missing_manifest_cannot_activate(self) -> None: - self.git("checkout", "-q", self.generated) - (self.repo / budget.MANIFEST.decode()).unlink() - checkpoint = self.commit(self.git("show", "-s", "--format=%B", self.generated)) - self.git("update-ref", "refs/remotes/origin/codegen/test", checkpoint) - self.git("checkout", "-q", "main") - stats = (self.repo / ".castiron.stats.yml").read_text() - self.write( - ".castiron.stats.yml", - stats.replace(self.generated, checkpoint).replace( - report.hash_codegen_commit(self.repo, self.generated), - report.hash_codegen_commit(self.repo, checkpoint), - ), - ) - self.assertEqual(self.evaluate()["checks"]["budget"]["state"], "failure") - - def test_legitimate_new_generated_checkpoint_passes(self) -> None: - self.git("checkout", "-q", self.generated) - self.write("generated.py", "new generated output\n") - self.write(self.workflow, "new generated workflow\n") - self.manifest() - checkpoint = self.commit(self.git("show", "-s", "--format=%B", self.generated)) - self.git("update-ref", "refs/remotes/origin/codegen/test", checkpoint) - self.git("checkout", "-q", "main") - for path in ("generated.py", self.workflow, budget.MANIFEST.decode()): - self.write(path, self.git("show", f"{checkpoint}:{path}") + "\n") - # Preserve exact manifest bytes (the fixture writes JSON without a newline). - self.write( - budget.MANIFEST.decode(), self.git("show", f"{checkpoint}:{budget.MANIFEST.decode()}") - ) - stats = (self.repo / ".castiron.stats.yml").read_text() - self.write( - ".castiron.stats.yml", - stats.replace(self.generated, checkpoint).replace( - report.hash_codegen_commit(self.repo, self.generated), - report.hash_codegen_commit(self.repo, checkpoint), - ), - ) - self.assertEqual(self.evaluate()["checks"]["budget"]["state"], "success") - - def test_worktree_verification_catches_uncommitted_edits(self) -> None: - self.assertEqual( - budget.verify_generated(self.repo, self.generated, self.base, worktree=True), [] - ) - self.write(self.workflow, "uncommitted\n") - failures = budget.verify_generated(self.repo, self.generated, self.base, worktree=True) - self.assertEqual(len(failures), 1) - self.assertIn(self.workflow, failures[0]) - - def test_activation_is_policy_only_and_checks_workflows(self) -> None: - strict = (self.repo / budget.POLICY).read_text() - self.write(budget.POLICY, '{"schema_version":1,"max_custom_patch_lines":1000}') - migration = self.commit() - self.write(budget.POLICY, strict) - activation = self.commit() - result, _ = budget.evaluate(self.repo, migration, activation, public=False) - self.assertTrue(all(check["state"] == "success" for check in result["checks"].values())) - self.write(self.workflow, "custom workflow\n") - result, _ = budget.evaluate(self.repo, migration, self.commit(), public=False) - self.assertTrue(all(check["state"] == "failure" for check in result["checks"].values())) - - def test_manifest_must_cover_snapshot_and_match_its_bytes(self) -> None: - self.git("checkout", "-q", self.generated) - manifest = json.loads((self.repo / budget.MANIFEST.decode()).read_text()) - manifest["files"].pop("generated.py") - self.write(budget.MANIFEST.decode(), json.dumps(manifest)) - with self.assertRaisesRegex(ValueError, "cover.*exactly"): - budget.generated_manifest(self.repo, self.commit()) - self.manifest() - self.write("generated.py", "snapshot changed without updating manifest\n") - with self.assertRaisesRegex(ValueError, "disagrees"): - budget.generated_manifest(self.repo, self.commit()) - - def test_relocated_tooling_preserves_ownership_and_retirement_checks(self) -> None: - self.git("checkout", "--detach", "-q", self.generated) - old_manifest = budget.MANIFEST.decode() - new_manifest = "generated/castiron/generated-files.json" - old_checker = "scripts/castiron/custom_code_budget.py" - new_checker = "generated/castiron/custom_code_budget.py" - manifest = json.loads((self.repo / old_manifest).read_text()) - manifest["files"][new_checker] = manifest["files"].pop(old_checker) - checker = (self.repo / old_checker).read_text() - self.write(new_checker, checker) - self.write(new_manifest, json.dumps(manifest)) - (self.repo / old_checker).unlink() - (self.repo / old_manifest).unlink() - relocated = self.commit("relocate generated tooling") - self.assertEqual( - budget.verify_generated(self.repo, relocated, relocated, previous=self.generated), [] - ) - ownership = budget.generated_manifest(self.repo, relocated) - self.assertIn(new_manifest.encode(), ownership) - self.assertIn(new_checker.encode(), ownership) - self.assertNotIn(old_manifest.encode(), ownership) - self.write(old_checker, checker) - self.write("generated.py", "handwritten modification\n") - failures = budget.verify_generated( - self.repo, relocated, self.commit(), previous=self.generated - ) - self.assertTrue( - any(old_checker in failure and "retired" in failure for failure in failures) - ) - self.assertTrue(any("generated.py: differs" in failure for failure in failures)) - # Candidate manifest edits cannot change the verified ownership contract. - self.write(new_manifest, '{"schema_version": 1, "files": {}}') - failures = budget.verify_generated(self.repo, relocated, self.commit()) - self.assertTrue( - any(new_manifest in failure and "differs" in failure for failure in failures) - ) - - def test_checkpoint_rejects_ambiguous_manifest_locations(self) -> None: - self.git("checkout", "--detach", "-q", self.generated) - self.write( - "generated/castiron/generated-files.json", - (self.repo / budget.MANIFEST.decode()).read_text(), - ) - with self.assertRaisesRegex(ValueError, "ambiguous"): - budget.generated_manifest(self.repo, self.commit()) - - def test_relocated_manifest_requires_colocated_checker_inventory(self) -> None: - self.git("checkout", "--detach", "-q", self.generated) - self.write( - "generated/castiron/generated-files.json", - (self.repo / budget.MANIFEST.decode()).read_text(), - ) - (self.repo / budget.MANIFEST.decode()).unlink() - with self.assertRaisesRegex(ValueError, "missing its generated GitHub inventory"): - budget.generated_manifest(self.repo, self.commit()) - - def test_manifest_preserves_inherited_sdk_owned_github_files(self) -> None: - self.git("checkout", "--detach", "-q", self.generated) - inherited = (".github/workflows/ci.yml", ".github/actions/setup-go/action.yml") - for path in inherited: - self.write(path, "SDK-owned configuration\n") - generated = self.commit("checkpoint preserves protected parent subtrees") - manifest = budget.generated_manifest(self.repo, generated) - for path in inherited: - self.assertNotIn(path.encode(), manifest) - self.write(path, "SDK-owned configuration update\n") - head = self.commit("handwritten configuration remains outside generated ownership") - self.assertEqual(budget.verify_generated(self.repo, generated, head), []) - - def test_private_manifest_checks_excluded_workflows_and_actions(self) -> None: - self.assertIn(self.workflow.encode(), budget.GENERATED_GITHUB_PATHS) - for path in (self.workflow, ".github/actions/generated/action.yml"): - for mutation in ("omit", "content", "mode"): - with ( - self.subTest(path=path, mutation=mutation), - mock.patch.object( - budget, - "GENERATED_GITHUB_PATHS", - budget.GENERATED_GITHUB_PATHS | {path.encode()}, - ), - ): - self.git("checkout", "--detach", "-q", self.generated) - self.write(path, "generated workflow or action\n") - self.inventory(budget.GENERATED_GITHUB_PATHS) - self.manifest((path,)) - manifest = json.loads((self.repo / budget.MANIFEST.decode()).read_text()) - manifest["files"][path] = { - "mode": "100644", - "sha256": hashlib.sha256((self.repo / path).read_bytes()).hexdigest(), - } - if mutation == "omit": - del manifest["files"][path] - elif mutation == "content": - self.write(path, "unrecorded change\n") - else: - (self.repo / path).chmod(0o755) - self.write(budget.MANIFEST.decode(), json.dumps(manifest)) - error = ( - "omits generated GitHub inventory path" - if mutation == "omit" - else "disagrees" - ) - with self.assertRaisesRegex(ValueError, error): - budget.generated_manifest(self.repo, self.commit()) - - def test_checkpoint_inventory_can_add_and_retire_generated_ci(self) -> None: - for path in (".github/workflows/new.yml", ".github/actions/new/action.yml"): - with self.subTest(path=path): - self.git("checkout", "--detach", "-q", self.generated) - self.write(path, "new generated CI\n") - self.inventory(budget.GENERATED_GITHUB_PATHS | {path.encode()}) - self.manifest((path,)) - newer = self.commit("new compiler inventory") - # The executing verifier still has the old inventory. - self.assertNotIn(path.encode(), budget.GENERATED_GITHUB_PATHS) - self.assertEqual( - budget.verify_generated(self.repo, newer, newer, previous=self.generated), [] - ) - (self.repo / path).unlink() - public = self.commit("public checkpoint omits private CI blob") - self.assertEqual( - budget.verify_generated(self.repo, public, newer, previous=self.generated), [] - ) - # A newer verifier must also read the prior inventory to enforce retirement. - self.assertEqual( - budget.verify_generated(self.repo, self.generated, self.base, previous=public), - [], - ) - self.git("checkout", "--detach", "-q", self.base) - self.write(path, "retired generated CI\n") - failures = budget.verify_generated( - self.repo, self.generated, self.commit(), previous=public - ) - self.assertTrue( - any(path in failure and "retired" in failure for failure in failures) - ) - - def test_manifest_cannot_claim_sdk_owned_workflows_or_actions(self) -> None: - for path in (".github/workflows/ci.yml", ".github/actions/setup-go/action.yml"): - for keep_blob in (True, False): - with self.subTest(path=path, keep_blob=keep_blob): - self.git("checkout", "--detach", "-q", self.generated) - self.write(path, "SDK-owned configuration\n") - manifest = json.loads((self.repo / budget.MANIFEST.decode()).read_text()) - manifest["files"][path] = { - "mode": "100644", - "sha256": hashlib.sha256((self.repo / path).read_bytes()).hexdigest(), - } - if not keep_blob: - (self.repo / path).unlink() - self.write(budget.MANIFEST.decode(), json.dumps(manifest)) - with self.assertRaisesRegex(ValueError, "SDK-owned GitHub path"): - budget.generated_manifest(self.repo, self.commit()) - - def test_public_snapshot_requires_every_inventory_identity(self) -> None: - for path in (self.workflow, ".github/actions/generated/action.yml"): - with self.subTest(path=path): - self.git("checkout", "--detach", "-q", self.generated) - self.write(path, "generated CI\n") - self.inventory(budget.GENERATED_GITHUB_PATHS | {path.encode()}) - self.manifest((path,)) - (self.repo / path).unlink() - manifest = json.loads((self.repo / budget.MANIFEST.decode()).read_text()) - del manifest["files"][path] - self.write(budget.MANIFEST.decode(), json.dumps(manifest)) - with self.assertRaisesRegex(ValueError, "omits generated GitHub inventory path"): - budget.generated_manifest( - self.repo, self.commit("incomplete public checkpoint") - ) - - def test_public_snapshot_keeps_strict_workflow_identity_without_private_blob(self) -> None: - self.git("checkout", "--detach", "-q", self.generated) - (self.repo / self.workflow).unlink() - public = self.commit("sanitized public checkpoint") - self.assertIn(self.workflow.encode(), budget.generated_manifest(self.repo, public)) - self.assertEqual(budget.verify_generated(self.repo, public, self.base), []) - self.git("checkout", "--detach", "-q", self.base) - self.write(self.workflow, "manual workflow edit\n") - violations = budget.verify_generated(self.repo, public, self.commit()) - self.assertEqual(len(violations), 1) - self.assertIn(self.workflow, violations[0]) - - def test_generated_deletion_requires_removing_retired_file(self) -> None: - self.git("checkout", "-q", self.generated) - (self.repo / "generated.py").unlink() - manifest = json.loads((self.repo / budget.MANIFEST.decode()).read_text()) - del manifest["files"]["generated.py"] - self.write(budget.MANIFEST.decode(), json.dumps(manifest)) - new = self.commit() - self.assertEqual(budget.verify_generated(self.repo, new, new, previous=self.generated), []) - self.write("generated.py", "retained handwritten replacement\n") - failures = budget.verify_generated(self.repo, new, self.commit(), previous=self.generated) - self.assertEqual(len(failures), 1) - self.assertIn("retired generated file", failures[0]) - - def test_generated_file_can_become_a_directory(self) -> None: - self.git("checkout", "-q", self.generated) - (self.repo / "generated.py").unlink() - self.write("generated.py/nested.py", "new generated file\n") - manifest = json.loads((self.repo / budget.MANIFEST.decode()).read_text()) - del manifest["files"]["generated.py"] - manifest["files"]["generated.py/nested.py"] = { - "mode": "100644", - "sha256": hashlib.sha256(b"new generated file\n").hexdigest(), - } - self.write(budget.MANIFEST.decode(), json.dumps(manifest)) - new = self.commit() - for worktree in (False, True): - with self.subTest(worktree=worktree): - self.assertEqual( - budget.verify_generated( - self.repo, new, new, previous=self.generated, worktree=worktree - ), - [], - ) - - def test_compiler_entrypoint_preserves_failed_worktree(self) -> None: - self.write(self.workflow, "uncommitted custom workflow\n") - result = subprocess.run( - [ - sys.executable, - "-I", - str(Path(budget.__file__)), - "verify-generated", - "--repo", - str(self.repo), - "--policy-ref", - self.base, - "--generated", - self.generated, - "--head", - self.base, - "--worktree", - ], - capture_output=True, - text=True, - check=False, - ) - self.assertEqual(result.returncode, 1) - self.assertIn(self.workflow, result.stderr) - self.assertEqual((self.repo / self.workflow).read_text(), "uncommitted custom workflow\n") - - if __name__ == "__main__": unittest.main() diff --git a/scripts/castiron/test_custom_code_github.py b/scripts/castiron/test_custom_code_github.py index 5e11fa4f09..4c71402e9d 100644 --- a/scripts/castiron/test_custom_code_github.py +++ b/scripts/castiron/test_custom_code_github.py @@ -14,7 +14,7 @@ import custom_code_budget as budget import custom_code_report as report -from custom_code_test_support import GENERATION, GitTestCase, source_run +from custom_code_test_support import GENERATION, GitTestCase, source_head, source_run REPOSITORY = "openai/example" @@ -28,7 +28,7 @@ def __init__(self, base: str, head: str, event: str = "pull_request") -> None: self.run = source_run(head, event) self.pull: dict[str, Any] = { "state": "open", - "head": {"sha": head}, + "head": source_head(head), "base": {"sha": base, "ref": "main", "repo": {"full_name": REPOSITORY}}, } self.associations = [{"number": 3}] @@ -68,7 +68,7 @@ def test_same_head_targets_preserve_required_main_discovery(self) -> None: run["pull_requests"] = [] main: dict[str, Any] = { "state": "open", - "head": {"sha": head}, + "head": source_head(head), "base": {"ref": "main", "repo": {"full_name": REPOSITORY}}, } next_pull: dict[str, Any] = {**main, "base": {**main["base"], "ref": "next"}} @@ -232,7 +232,7 @@ def test_fork_association_fallback_paginates_candidates(self) -> None: run: dict[str, Any] = { "head_sha": "a" * 40, "pull_requests": [], - "head_repository": {"owner": {"login": "contributor"}}, + "head_repository": source_run("a" * 40)["head_repository"], "head_branch": "fix/branch&other=value", } first = [{"number": number} for number in range(1, 101)] @@ -620,6 +620,65 @@ def local_fetch(repo: Path, *args: str, input_bytes: bytes | None = None) -> byt [c["state"] for c in result["checks"].values()], ["success", "success"] ) + def test_same_commit_discovery_is_bound_to_source_repository_and_branch( + self, + ) -> None: + run = source_run("b" * 40) + run["pull_requests"] = [] + own: dict[str, Any] = { + "state": "open", + "head": source_head(run["head_sha"]), + "base": {"ref": "main", "repo": {"full_name": REPOSITORY}}, + } + for foreign_head in ( + {**own["head"], "ref": "other"}, + {**own["head"], "repo": {"id": 11, "full_name": "victim/example"}}, + {**own["head"], "repo": None}, + ): + victim = {**own, "head": foreign_head} + + def discover(_method: str, path: str, victim: dict[str, Any] = victim) -> Any: + if "/commits/" in path: + return [{"number": 3}, {"number": 4}] + return {"3": own, "4": victim}[path.rsplit("/", 1)[1]] + + with ( + self.subTest(head=foreign_head), + mock.patch.object( + report, + "api", + side_effect=discover, + ), + ): + self.assertEqual(report.current_pull(REPOSITORY, run), (3, "main")) + + def test_foreign_only_associations_retry_source_branch(self) -> None: + run = source_run("b" * 40) + run["pull_requests"] = [] + own: dict[str, Any] = { + "state": "open", + "head": source_head("b" * 40), + "base": {"ref": "main", "repo": {"full_name": REPOSITORY}}, + } + other = {**own, "head": {**own["head"], "ref": "other"}} + branches = [{"number": 3}] + + def api(method: str, path: str) -> Any: + if "/commits/" in path: + return [{"number": 4}] + if "/pulls?" in path: + return branches + return {"3": own, "4": other, "5": own}[path.rsplit("/", 1)[1]] + + with mock.patch.object(report, "api", side_effect=api): + self.assertEqual(report.current_pull(REPOSITORY, run), (3, "main")) + run["pull_requests"] = [{"number": 4}] + self.assertIsNone(report.current_pull(REPOSITORY, run)) + run["pull_requests"] = [] + branches.append({"number": 5}) + with self.assertRaisesRegex(report.ReportError, "multiple current"): + report.current_pull(REPOSITORY, run) + class GitHubBudgetTests(unittest.TestCase): def setUp(self) -> None: # pyright: ignore[reportImplicitOverride] @@ -962,6 +1021,8 @@ def test_github_uses_fresh_bare_data_repo_and_checks_current_head(self) -> None: measurement=None, policy_sha=self.base, target_branch="main", + main_snapshot=None, + repository=REPOSITORY, ) def test_stale_pull_and_wrong_target_fail_before_objects_created(self) -> None: diff --git a/scripts/castiron/test_custom_code_policy.py b/scripts/castiron/test_custom_code_policy.py index 5368d30ca7..d71aef3e12 100644 --- a/scripts/castiron/test_custom_code_policy.py +++ b/scripts/castiron/test_custom_code_policy.py @@ -3,7 +3,6 @@ from __future__ import annotations -import copy import io import json import sys @@ -120,6 +119,37 @@ def api(method: str, path: str) -> Any: return api + def test_policy_review_fork_fallback_keeps_live_exact_head_checks(self) -> None: + original = self.review_api([self.approved()]) + current = original("GET", "repos/openai/example/pulls/3") + cases: list[tuple[dict[str, Any], bool]] = [ + ({}, True), + ({"state": "closed"}, False), + ({"head": {"sha": "f" * 40}}, False), + ({"base": {"ref": "main", "repo": {"full_name": REPO}}}, False), + ({"base": {"ref": "next", "repo": {"full_name": "other/repo"}}}, False), + ] + for change, accepted in cases: + with self.subTest(change=change): + + def api(method: str, path: str, change: dict[str, Any] = change) -> Any: + if "/commits/" in path: + return [] + if "/pulls?" in path: + if path.endswith("page=1"): + return [{"head": {"sha": "a" * 40}}] * 100 + return [current] + if path.endswith("/pulls/3"): + return {**current, **change} + return original(method, path) + + with mock.patch.object(selector.report, "api", side_effect=api): + if accepted: + self.assertEqual(selector.require_human_policy_review(REPO, self.head), 3) + else: + with self.assertRaisesRegex(ValueError, "one open next PR"): + selector.require_human_policy_review(REPO, self.head) + def approved(self, **kwargs: Any) -> dict[str, Any]: return { "id": 10, @@ -138,7 +168,11 @@ def test_fork_policy_review_uses_the_discovered_pull(self) -> None: review = self.review_api([self.approved()]) run: dict[str, Any] = { "head_sha": self.head, - "head_repository": {"owner": {"login": "forker"}}, + "head_repository": { + "id": 17, + "full_name": "forker/example", + "owner": {"login": "forker"}, + }, "head_branch": "policy", "pull_requests": [], } @@ -149,7 +183,10 @@ def api(method: str, path: str) -> Any: if "/pulls?" in path: self.assertIn("head=forker%3Apolicy", path) return [{"number": 3}] - return review(method, path) + result = review(method, path) + if path == f"repos/{REPO}/pulls/3": + result["head"].update(ref="policy", repo=run["head_repository"]) + return result with mock.patch.object(selector.report, "api", side_effect=api): current = selector.report.current_pull(REPO, run) @@ -246,6 +283,8 @@ def test_queue_bootstrap_isolated_per_constituent(self) -> None: self.select(base=self.main, head=group, pull_heads=heads) with self.assertRaisesRegex(ValueError, "verified constituent"): self.select(base=self.main, head=group, pull_heads=[]) + with self.assertRaisesRegex(ValueError, "verified policy-only constituent"): + self.select(base=self.main, head=group, pull_heads=[sdk_head]) def test_queue_pin_update_isolated_per_constituent(self) -> None: self.git("checkout", "-q", "--detach", self.main) @@ -782,169 +821,130 @@ def test_main_selection_unchanged(self) -> None: self.assertEqual(result["policy_sha"], self.main) self.assertFalse(result["bootstrap"]) - -POLICY_SOURCE_ROOT = "repos/" + REPO -CANONICAL_POLICY_WORKFLOW = ".github/workflows/castiron-custom-code.yml" -CI_POLICY_WORKFLOW = ".github/workflows/ci.yml" - - -class PolicySourceWorkflowTests(unittest.TestCase): - def setUp(self) -> None: # pyright: ignore[reportImplicitOverride] - self.temp = tempfile.TemporaryDirectory() - self.addCleanup(self.temp.cleanup) - self.repo = Path(self.temp.name) / "objects" - self.workflow_run: dict[str, Any] = { - "id": 42, - "workflow_id": 456, - "repository": {"full_name": REPO}, - "head_sha": "a" * 40, - "status": "completed", - "event": "pull_request", - "run_attempt": 3, - "path": CANONICAL_POLICY_WORKFLOW + "@refs/pull/12/merge", - } - self.event: dict[str, Any] = { - "repository": {"full_name": REPO}, - "workflow_run": copy.deepcopy(self.workflow_run), + def prepare_snapshot(self) -> None: + self.git("checkout", "-q", "--detach", self.main) + self.base = self.main # unpinned legacy next + self.strict = { + "schema_version": 2, + "max_custom_patch_lines": 0, + "generated_files": "strict", } - self.branch = "next" - self.select = self.start_patch( - mock.patch.object(selector, "select_policy", return_value={"selected": True}) + self.write_policy(self.strict) + self.write("sdk.txt", "approved main output\n") + self.write( + ".castiron.stats.yml", + "schema_version: 1\ngeneration_id: 550e8400-e29b-41d4-a716-446655440000\ncodegen_sha: " + + "a" * 40 + + "\npublic_codegen_sha: " + + "a" * 40 + + "\n", ) - self.start_patch(mock.patch.object(selector.report, "api", side_effect=self.api)) - self.start_patch( - mock.patch.object(selector.report, "current_pull", side_effect=self.current_pull) + self.main = self.commit("strict main snapshot") + self.git("update-ref", "refs/remotes/origin/main", self.main) + self.write_policy( + {**self.strict, "next_policy": {"repository": REPO, "main_sha": self.main}} ) - self.start_patch( - mock.patch.object( - selector.report, - "git", - side_effect=self.git_result, - ) + tree_commit = self.commit("adopt exact main snapshot") + self.head = self.git( + "commit-tree", + f"{tree_commit}^{{tree}}", + "-p", + self.base, + "-p", + self.main, + "-m", + "canonical snapshot", ) + self.git("checkout", "--detach", "-q", self.head) - def current_pull(self, *args: Any) -> tuple[int, str]: - return 12, self.branch - - def git_result(self, repo: Path, *args: str) -> bytes: - return b"b" * 40 if args[0] == "rev-parse" else b"" - - def start_patch(self, patcher: Any) -> Any: - result = patcher.start() - self.addCleanup(patcher.stop) - return result - - def api(self, method: str, path: str, *args: Any) -> Any: - return { - POLICY_SOURCE_ROOT: {"default_branch": "main"}, - POLICY_SOURCE_ROOT + "/actions/runs/42": self.workflow_run, - POLICY_SOURCE_ROOT + "/actions/workflows/castiron-custom-code.yml": { - "id": 456, - "path": CANONICAL_POLICY_WORKFLOW, - }, - POLICY_SOURCE_ROOT + "/actions/workflows/ci.yml": { - "id": 456, - "path": CI_POLICY_WORKFLOW, - }, - POLICY_SOURCE_ROOT + "/git/ref/heads/main": {"object": {"sha": "c" * 40}}, - POLICY_SOURCE_ROOT + "/git/ref/heads/next": {"object": {"sha": "b" * 40}}, - }[path] - - def call(self, **kwargs: Any) -> dict[str, Any]: - return selector.select_github_policy(self.repo, REPO, self.event, "c" * 40, **kwargs) - - def use_ci(self) -> None: - self.workflow_run["path"] = CI_POLICY_WORKFLOW + "@refs/pull/12/merge" - - def test_lookalike_workflow_cannot_select_policy(self) -> None: - self.workflow_run["path"] = CANONICAL_POLICY_WORKFLOW + "@evil.yml" - self.workflow_run["workflow_id"] = 789 - with self.assertRaisesRegex(ValueError, "canonical workflow"): - self.call() - self.select.assert_not_called() - - def test_default_canonical_unchanged(self) -> None: - self.branch = "main" - self.assertEqual(self.call(), {"selected": True}) - - def test_explicit_ci_uses_same_base_pin_selector(self) -> None: - self.use_ci() - self.assertEqual(self.call(source_workflow=CI_POLICY_WORKFLOW), {"selected": True}) - self.select.assert_called_once_with( - self.repo, - REPO, - "next", - "b" * 40, - "a" * 40, - "c" * 40, - pull_heads=None, - pull_numbers={"a" * 40: 12}, + def test_exact_main_snapshot_pr_and_queue(self) -> None: + self.prepare_snapshot() + for queue in (False, True): + with ( + self.subTest(queue=queue), + mock.patch.object( + selector.report, "api", side_effect=AssertionError("no human proof needed") + ), + ): + receipt = self.select(pull_heads=[self.head] if queue else None) + self.assertEqual(receipt["main_snapshot"], self.main) + self.assertEqual(receipt["policy_sha"], self.main) + self.assertEqual(receipt["policy_reviews"], []) + + def test_snapshot_rejects_additional_bytes_modes_or_metadata(self) -> None: + self.prepare_snapshot() + valid = self.head + for kind in ("content", "mode", "extra", "stats", "symlink"): + for queue in (False, True): + with self.subTest(kind=kind, queue=queue): + self.git("reset", "--hard", valid) + if kind == "content": + self.write("sdk.txt", "unreviewed output\n") + elif kind == "mode": + (self.repo / "sdk.txt").chmod(0o755) + elif kind == "extra": + self.write("extra.txt", "extra") + elif kind == "stats": + self.write(".castiron.stats.yml", "schema_version: 1\n") + else: + (self.repo / "sdk.txt").unlink() + (self.repo / "sdk.txt").symlink_to(".castiron.stats.yml") + self.head = self.commit("invalid adoption") + with self.assertRaises(ValueError): + self.select(pull_heads=[self.head] if queue else None) + + def test_snapshot_rejects_main_movement_and_nonancestor_target(self) -> None: + self.prepare_snapshot() + old_main = self.main + candidate = self.head + self.git("checkout", "-q", "--detach", old_main) + self.write("sdk.txt", "new approved main\n") + self.main = self.commit("main advances") + self.git("update-ref", "refs/remotes/origin/main", self.main) + with self.assertRaises(ValueError): + self.select(head=candidate) + self.main = old_main + self.git("update-ref", "refs/remotes/origin/main", self.main) + self.git("checkout", "-q", "--detach", self.base) + self.write("private-only.txt", "unmerged next history") + self.base = self.commit("divergent next") + with self.assertRaises(ValueError): + self.select(head=candidate) + + def test_snapshot_rejects_private_history_but_accepts_verified_queue_wrapper(self) -> None: + self.prepare_snapshot() + constituent = self.head + wrapper = self.git( + "commit-tree", + f"{constituent}^{{tree}}", + "-p", + self.base, + "-p", + constituent, + "-m", + "queue wrapper", ) - - def test_candidate_event_cannot_select_ci_adapter(self) -> None: - self.use_ci() - self.event["source_workflow"] = CI_POLICY_WORKFLOW - with self.assertRaisesRegex(ValueError, "source run"): - self.call() - self.select.assert_not_called() - - def test_unknown_workflow_rejected(self) -> None: - with self.assertRaisesRegex(ValueError, "source workflow"): - self.call(source_workflow=".github/workflows/evil.yml") - self.select.assert_not_called() - - def test_ci_adapter_cannot_retarget_main(self) -> None: - self.use_ci() - self.branch = "main" - with self.assertRaisesRegex(ValueError, "restricted to next"): - self.call(source_workflow=CI_POLICY_WORKFLOW) - self.select.assert_not_called() - - def test_wrong_actual_workflow(self) -> None: - with self.assertRaisesRegex(ValueError, "source run"): - self.call(source_workflow=CI_POLICY_WORKFLOW) - self.select.assert_not_called() - - def test_stale_or_mismatched_authenticated_identity(self) -> None: - self.use_ci() - for field, value in [ - ("head_sha", "d" * 40), - ("run_attempt", 4), - ("status", "in_progress"), - ("event", "push"), - ("repository", {"full_name": "attacker/example"}), - ]: - with self.subTest(field=field): - old = self.workflow_run[field] - self.workflow_run[field] = value - with self.assertRaisesRegex(ValueError, "source run"): - self.call(source_workflow=CI_POLICY_WORKFLOW) - self.workflow_run[field] = old - self.select.assert_not_called() - - -class SelectorInterfaceTests(unittest.TestCase): - def test_budget_cli_forwards_to_adjacent_selector(self) -> None: - args = ["select-policy", "--repo", "objects", "--repository", REPO] - with ( - mock.patch.object(sys, "argv", ["budget", *args]), - mock.patch.object(budget.policy, "main", return_value=0) as select, - ): - self.assertEqual(budget.main(), 0) - select.assert_called_once_with(args) - - def test_isolated_cli_loads_only_adjacent_modules(self) -> None: - import subprocess - - for module in (budget, selector): - assert module.__file__ is not None - completed = subprocess.run( - [sys.executable, "-I", module.__file__, "select-policy", "--help"], - capture_output=True, - text=True, - check=True, - ) - self.assertIn("--source-workflow", completed.stdout) + self.assertEqual( + self.select(head=wrapper, pull_heads=[constituent])["main_snapshot"], self.main + ) + with self.assertRaises(ValueError): + self.select(head=wrapper) # A wrapper is not an independently verified PR head. + self.write("private.txt", "private-only historical bytes") + self.commit("private historical commit") + (self.repo / "private.txt").unlink() + reverted = self.commit("revert private historical bytes") + self.assertEqual( + self.git("rev-parse", f"{reverted}^{{tree}}"), + self.git("rev-parse", f"{constituent}^{{tree}}"), + ) + for heads in (None, [reverted]): + with self.subTest(queue=heads is not None), self.assertRaises(ValueError): + self.select(head=reverted, pull_heads=heads) + + def test_snapshot_rejects_multiple_queue_constituents(self) -> None: + self.prepare_snapshot() + with self.assertRaisesRegex(ValueError, "one exact"): + self.select(pull_heads=[self.head, self.head]) if __name__ == "__main__": diff --git a/scripts/castiron/test_custom_code_policy_source.py b/scripts/castiron/test_custom_code_policy_source.py new file mode 100644 index 0000000000..1b60637c45 --- /dev/null +++ b/scripts/castiron/test_custom_code_policy_source.py @@ -0,0 +1,198 @@ +# File generated from our OpenAPI spec by Castiron. See CONTRIBUTING.md for details. +"""Current-main policy selection and review authorization regressions.""" + +from __future__ import annotations + +import copy +import json +import sys +import tempfile +import unittest +import unittest.mock as mock # noqa: PLR0402 - keep unittest imports consistent +from pathlib import Path +from typing import Any + +import custom_code_budget as budget +import custom_code_policy as selector + +REPO = "openai/example" + + +POLICY_SOURCE_ROOT = "repos/" + REPO +CANONICAL_POLICY_WORKFLOW = ".github/workflows/castiron-custom-code.yml" +CI_POLICY_WORKFLOW = ".github/workflows/ci.yml" + + +class PolicySourceWorkflowTests(unittest.TestCase): + def setUp(self) -> None: # pyright: ignore[reportImplicitOverride] + self.temp = tempfile.TemporaryDirectory() + self.addCleanup(self.temp.cleanup) + self.repo = Path(self.temp.name) / "objects" + self.workflow_run: dict[str, Any] = { + "id": 42, + "workflow_id": 456, + "repository": {"full_name": REPO}, + "head_sha": "a" * 40, + "status": "completed", + "event": "pull_request", + "run_attempt": 3, + "path": CANONICAL_POLICY_WORKFLOW + "@refs/pull/12/merge", + } + self.event: dict[str, Any] = { + "repository": {"full_name": REPO}, + "workflow_run": copy.deepcopy(self.workflow_run), + } + self.branch = "next" + self.select = self.start_patch( + mock.patch.object(selector, "select_policy", return_value={"selected": True}) + ) + self.start_patch(mock.patch.object(selector.report, "api", side_effect=self.api)) + self.start_patch( + mock.patch.object(selector.report, "current_pull", side_effect=self.current_pull) + ) + self.start_patch( + mock.patch.object( + selector.report, + "git", + side_effect=self.git_result, + ) + ) + + def current_pull(self, *args: Any) -> tuple[int, str]: + return 12, self.branch + + def git_result(self, repo: Path, *args: str) -> bytes: + return b"b" * 40 if args[0] == "rev-parse" else b"" + + def start_patch(self, patcher: Any) -> Any: + result = patcher.start() + self.addCleanup(patcher.stop) + return result + + def api(self, method: str, path: str, *args: Any) -> Any: + return { + POLICY_SOURCE_ROOT: {"default_branch": "main"}, + POLICY_SOURCE_ROOT + "/actions/runs/42": self.workflow_run, + POLICY_SOURCE_ROOT + "/actions/workflows/castiron-custom-code.yml": { + "id": 456, + "path": CANONICAL_POLICY_WORKFLOW, + }, + POLICY_SOURCE_ROOT + "/actions/workflows/ci.yml": { + "id": 456, + "path": CI_POLICY_WORKFLOW, + }, + POLICY_SOURCE_ROOT + "/git/ref/heads/main": {"object": {"sha": "c" * 40}}, + POLICY_SOURCE_ROOT + "/git/ref/heads/next": {"object": {"sha": "b" * 40}}, + }[path] + + def call(self, **kwargs: Any) -> dict[str, Any]: + return selector.select_github_policy(self.repo, REPO, self.event, "c" * 40, **kwargs) + + def use_ci(self) -> None: + self.workflow_run["path"] = CI_POLICY_WORKFLOW + "@refs/pull/12/merge" + + def test_lookalike_workflow_cannot_select_policy(self) -> None: + self.workflow_run["path"] = CANONICAL_POLICY_WORKFLOW + "@evil.yml" + self.workflow_run["workflow_id"] = 789 + with self.assertRaisesRegex(ValueError, "canonical workflow"): + self.call() + self.select.assert_not_called() + + def test_default_canonical_unchanged(self) -> None: + self.branch = "main" + self.assertEqual(self.call(), {"selected": True}) + + def test_explicit_ci_uses_same_base_pin_selector(self) -> None: + self.use_ci() + self.assertEqual(self.call(source_workflow=CI_POLICY_WORKFLOW), {"selected": True}) + self.select.assert_called_once_with( + self.repo, + REPO, + "next", + "b" * 40, + "a" * 40, + "c" * 40, + pull_heads=None, + pull_numbers={"a" * 40: 12}, + ) + + def test_candidate_event_cannot_select_ci_adapter(self) -> None: + self.use_ci() + self.event["source_workflow"] = CI_POLICY_WORKFLOW + with self.assertRaisesRegex(ValueError, "source run"): + self.call() + self.select.assert_not_called() + + def test_unknown_workflow_rejected(self) -> None: + with self.assertRaisesRegex(ValueError, "source workflow"): + self.call(source_workflow=".github/workflows/evil.yml") + self.select.assert_not_called() + + def test_ci_adapter_cannot_retarget_main(self) -> None: + self.use_ci() + self.branch = "main" + with self.assertRaisesRegex(ValueError, "restricted to next"): + self.call(source_workflow=CI_POLICY_WORKFLOW) + self.select.assert_not_called() + + def test_wrong_actual_workflow(self) -> None: + with self.assertRaisesRegex(ValueError, "source run"): + self.call(source_workflow=CI_POLICY_WORKFLOW) + self.select.assert_not_called() + + def test_stale_or_mismatched_authenticated_identity(self) -> None: + self.use_ci() + for field, value in [ + ("head_sha", "d" * 40), + ("run_attempt", 4), + ("status", "in_progress"), + ("event", "push"), + ("repository", {"full_name": "attacker/example"}), + ]: + with self.subTest(field=field): + old = self.workflow_run[field] + self.workflow_run[field] = value + with self.assertRaisesRegex(ValueError, "source run"): + self.call(source_workflow=CI_POLICY_WORKFLOW) + self.workflow_run[field] = old + self.select.assert_not_called() + + +class SelectorInterfaceTests(unittest.TestCase): + def test_review_rules_cli_exports_the_canonical_predicate(self) -> None: + import subprocess + + assert budget.__file__ is not None + completed = subprocess.run( + [sys.executable, "-I", budget.__file__, "review-rules"], + capture_output=True, + text=True, + check=True, + ) + self.assertEqual(json.loads(completed.stdout), selector.HUMAN_REVIEW_RULES) + + def test_budget_cli_forwards_to_adjacent_selector(self) -> None: + args = ["select-policy", "--repo", "objects", "--repository", REPO] + with ( + mock.patch.object(sys, "argv", ["budget", *args]), + mock.patch.object(budget.policy, "main", return_value=0) as select, + ): + self.assertEqual(budget.main(), 0) + select.assert_called_once_with(args) + + def test_isolated_cli_loads_only_adjacent_modules(self) -> None: + import subprocess + + for module in (budget, selector): + assert module.__file__ is not None + completed = subprocess.run( + [sys.executable, "-I", module.__file__, "select-policy", "--help"], + capture_output=True, + text=True, + check=True, + ) + self.assertIn("--source-workflow", completed.stdout) + + +if __name__ == "__main__": + unittest.main() diff --git a/scripts/castiron/test_custom_code_publication.py b/scripts/castiron/test_custom_code_publication.py index 4387e6cd9f..b56cce0d9b 100644 --- a/scripts/castiron/test_custom_code_publication.py +++ b/scripts/castiron/test_custom_code_publication.py @@ -18,6 +18,7 @@ GitTestCase, run_publisher, run_workflow_script, + source_head, source_run, ) @@ -47,7 +48,7 @@ def test_workflow_path_filters_accept_refs_without_accepting_other_files(self) - pull = { "number": 3, "state": "open", - "head": {"sha": head}, + "head": source_head(head), "base": {"ref": "next", "repo": {"full_name": "openai/example"}}, } for path, accepted in cases: @@ -100,7 +101,7 @@ def test_trusted_failure_publisher_updates_one_current_comment(self) -> None: current = { "number": 3, "state": "open", - "head": {"sha": head}, + "head": source_head(head), "base": {"ref": "main", "repo": {"full_name": "openai/example"}}, } previous = { @@ -151,7 +152,7 @@ def pull(number: int, branch: str = "main", repo: str = "openai/example") -> dic return { "number": number, "state": "open", - "head": {"sha": head}, + "head": source_head(head), "base": {"ref": branch, "repo": {"full_name": repo}}, } @@ -223,6 +224,95 @@ def test_workflow_reports_all_branches_without_write_credentials(self) -> None: ).hexdigest() self.assertIn(f"REPORTER_SHA256: {digest}", producer) + @unittest.skipUnless(shutil.which("node"), "GitHub Actions JavaScript runtime") + def test_review_signals_cannot_replace_pending_candidate_validation(self) -> None: + producer = (WORKFLOW.parent / "castiron-custom-code.yml").read_text() + group = producer.split("concurrency:\n", 1)[1].splitlines()[0].removeprefix(" group: ") + expression = group.replace("${{", "${").replace("}}", "}") + groups: list[str] = [] + for event in ("pull_request", "pull_request_review", "pull_request"): + payload = {"context": {"payload": {"pull_request": {"number": 42}}}} + script = ( + "github.event = context.payload;\n" + f"github.event_name = {json.dumps(event)};\n" + f"await github.rest.issues.createComment({{body: `{expression}`}});" + ) + groups.append(run_workflow_script(script, payload)[0]["body"]) + self.assertNotEqual(groups[0], groups[1]) + self.assertEqual(groups[0], groups[2]) + + @unittest.skipUnless(shutil.which("node"), "GitHub Actions JavaScript runtime") + def test_failure_publisher_binds_one_source_before_writing(self) -> None: + head = "a" * 40 + run: dict[str, Any] = {**source_run(head), "id": 20, "pull_requests": []} + own: dict[str, Any] = { + "number": 3, + "state": "open", + "head": source_head(head), + "base": {"ref": "main", "repo": {"full_name": "openai/example"}}, + } + previous = { + "id": 42, + "user": {"type": "Bot", "login": "github-actions[bot]"}, + "body": report.MARKER + "\n", + } + + def publish(**overrides: Any) -> list[dict[str, Any]]: + return run_publisher( + "Publish a trusted failure status", + { + "context": { + "payload": {"workflow_run": run}, + "runId": 30, + "repo": {"owner": "openai", "repo": "example"}, + "serverUrl": "https://github.com", + }, + "run": run, + "current": own, + "comments": [previous], + "comments_by_number": {3: [previous], 4: [{**previous, "id": 43}]}, + "associated_pulls": [{"number": 3}, {"number": 4}], + **overrides, + }, + ) + + for foreign_head in ( + {**own["head"], "ref": "victim"}, + {**own["head"], "repo": {"id": 11, "full_name": "victim/example"}}, + ): + victim = {**own, "number": 4, "head": foreign_head} + with self.subTest(head=foreign_head): + writes = publish(pulls_by_number={3: own, 4: victim}) + self.assertEqual([write["comment_id"] for write in writes], [42]) + writes = publish( + associated_pulls=[{"number": 4}], + fallback_pulls=[{"number": 3}], + pulls_by_number={3: own, 4: victim}, + ) + self.assertEqual([write["comment_id"] for write in writes], [42]) + self.assertEqual( + publish( + run={**run, "pull_requests": [{"number": 4}]}, + fallback_pulls=[{"number": 3}], + pulls_by_number={3: own, 4: victim}, + ), + [], + ) + # Even two equally authorized PRs are ambiguous: no write to either. + self.assertEqual(publish(), []) + for changed_run in ( + {**run, "run_attempt": 2}, + {**run, "status": "in_progress"}, + {**run, "head_repository": None}, + {**run, "repository": {"full_name": "other/repo"}}, + ): + with self.subTest(run=changed_run): + self.assertEqual(publish(run=changed_run), []) + changed = {**own, "head": {**own["head"], "ref": "other"}} + self.assertEqual( + publish(associated_pulls=[{"number": 3}], current_sequence=[own, changed]), [] + ) + class CommentTests(GitTestCase): def test_comment_only_rerun_links_to_the_compute_artifact_attempt(self) -> None: @@ -264,10 +354,11 @@ def resolve(value: str, context: dict[str, str]) -> str: result, _ = report.build_report(self.repo, base, base) pull: dict[str, Any] = { "state": "open", - "head": {"sha": base}, + "head": source_head(base), "base": {"sha": base, "ref": "main", "repo": {"full_name": "openai/example"}}, } run = { + **source_run(base), "event": "pull_request", "path": ".github/workflows/castiron-custom-code.yml", "head_sha": base, @@ -275,7 +366,7 @@ def resolve(value: str, context: dict[str, str]) -> str: "pull_requests": [{"number": 1}], } with mock.patch.object( - report, "api", side_effect=[pull, run, [], pull, {"html_url": "published"}] + report, "api", side_effect=[pull, run, pull, [], pull, {"html_url": "published"}] ) as api: self.assertEqual( report.publish_comment( @@ -304,11 +395,16 @@ def fake_api(method: str, path: str, payload: object = None) -> object: if "/pulls/" in path: return { "state": "open", - "head": {"sha": base}, - "base": {"sha": base, "ref": "main", "repo": {"full_name": "openai/example"}}, + "head": source_head(base), + "base": { + "sha": base, + "ref": "main", + "repo": {"full_name": "openai/example"}, + }, } if "/actions/runs/" in path: return { + **source_run(base), "event": "pull_request", "path": ".github/workflows/castiron-custom-code.yml", "head_sha": base, @@ -333,7 +429,8 @@ def fake_api(method: str, path: str, payload: object = None) -> object: calls.clear() result["head_sha"] = "f" * 40 self.assertEqual( - report.publish_comment(result, "openai/example", 1, 2, 1), "Skipped stale report" + report.publish_comment(result, "openai/example", 1, 2, 1), + "Skipped stale report", ) self.assertEqual(len(calls), 1) @@ -342,17 +439,18 @@ def test_comment_rejects_older_runs_attempts_and_wrong_pr(self) -> None: result, _ = report.build_report(self.repo, base, base) pull: dict[str, Any] = { "state": "open", - "head": {"sha": base}, + "head": source_head(base), "base": {"sha": base, "ref": "main", "repo": {"full_name": "openai/example"}}, } run = { + **source_run(base), "event": "pull_request", "path": ".github/workflows/castiron-custom-code.yml", "head_sha": base, "run_attempt": 2, "pull_requests": [{"number": 1}], - "head_repository": {"owner": {"login": "contributor"}}, - "head_branch": "fix/branch", + "head_repository": source_run(base)["head_repository"], + "head_branch": "sdk", } comment = { "id": 8, @@ -362,17 +460,19 @@ def test_comment_rejects_older_runs_attempts_and_wrong_pr(self) -> None: } with mock.patch.object(report, "api", side_effect=[pull, run]) as api: self.assertEqual( - report.publish_comment(result, "openai/example", 1, 2, 1), "Skipped stale report" + report.publish_comment(result, "openai/example", 1, 2, 1), + "Skipped stale report", ) self.assertEqual(api.call_count, 2) - with mock.patch.object(report, "api", side_effect=[pull, run, [comment]]) as api: + with mock.patch.object(report, "api", side_effect=[pull, run, pull, [comment]]) as api: self.assertEqual( - report.publish_comment(result, "openai/example", 1, 2, 2), "Skipped stale report" + report.publish_comment(result, "openai/example", 1, 2, 2), + "Skipped stale report", ) - self.assertEqual(api.call_count, 3) + self.assertEqual(api.call_count, 4) with ( mock.patch.object( - report, "api", side_effect=[pull, {**run, "pull_requests": []}, [], []] + report, "api", side_effect=[pull, {**run, "pull_requests": []}, [], [], []] ), self.assertRaisesRegex(report.ReportError, "does not match report PR"), ): @@ -385,30 +485,117 @@ def test_comment_rejects_older_runs_attempts_and_wrong_pr(self) -> None: with mock.patch.object( report, "api", - side_effect=[pull, {**run, "pull_requests": []}, [{"number": 1}], [comment]], + side_effect=[pull, {**run, "pull_requests": []}, [{"number": 1}], pull, [comment]], ) as api: self.assertEqual( - report.publish_comment(result, "openai/example", 1, 2, 2), "Skipped stale report" + report.publish_comment(result, "openai/example", 1, 2, 2), + "Skipped stale report", ) self.assertIn(f"/commits/{base}/pulls", api.call_args_list[2].args[1]) - changed = {**pull, "head": {"sha": "f" * 40}} - with mock.patch.object(report, "api", side_effect=[pull, run, [], changed]) as api: + changed = {**pull, "head": source_head("f" * 40)} + with mock.patch.object(report, "api", side_effect=[pull, run, pull, [], changed]) as api: self.assertEqual( - report.publish_comment(result, "openai/example", 1, 2, 2), "Skipped stale report" + report.publish_comment(result, "openai/example", 1, 2, 2), + "Skipped stale report", ) - self.assertEqual(api.call_count, 4) + self.assertEqual(api.call_count, 5) for target in ( {**pull["base"], "ref": "other"}, {**pull["base"], "repo": {"full_name": "other/repo"}}, ): with mock.patch.object( - report, "api", side_effect=[pull, run, [], {**pull, "base": target}] + report, "api", side_effect=[pull, run, pull, [], {**pull, "base": target}] ): self.assertEqual( report.publish_comment(result, "openai/example", 1, 2, 2), "Skipped stale report", ) + def test_comment_uses_canonical_branch_fallback(self) -> None: + run = source_run("a" * 40) + run["pull_requests"] = [] + own: dict[str, Any] = { + "state": "open", + "head": source_head("a" * 40), + "base": {"ref": "main", "repo": {"full_name": "openai/example"}}, + } + foreign = {**own, "head": {**own["head"], "ref": "other"}} + branches = [{"number": 3}] + writes: list[str] = [] + measured = { + "schema_version": 1, + "status": "error", + "head_sha": "a" * 40, + "target_base_sha": "b" * 40, + "error": "synthetic failure", + } + + def api(method: str, path: str, payload: Any = None) -> Any: + if method == "POST": + writes.append(path) + return {"html_url": "published"} + if "/actions/runs/" in path: + return run + if "/commits/" in path: + return [{"number": 4}] + if "/pulls?" in path: + return branches + if "/comments?" in path: + return [] + return {"3": own, "4": foreign, "5": own}[path.rsplit("/", 1)[1]] + + with mock.patch.object(report, "api", side_effect=api): + self.assertEqual(report.current_pull("openai/example", run), (3, "main")) + self.assertEqual( + report.publish_comment(measured, "openai/example", 3, 123, 1), "published" + ) + self.assertEqual(writes, ["repos/openai/example/issues/3/comments"]) + writes.clear() + run["pull_requests"] = [{"number": 4}] + with self.assertRaisesRegex(report.ReportError, "does not match report PR"): + report.publish_comment(measured, "openai/example", 3, 123, 1) + run["pull_requests"] = [] + branches.append({"number": 5}) + with self.assertRaisesRegex(report.ReportError, "multiple current"): + report.publish_comment(measured, "openai/example", 3, 123, 1) + self.assertEqual(writes, []) + + def test_successful_comment_rejects_another_source_before_and_after_pagination( + self, + ) -> None: + head = "a" * 40 + run = source_run(head) + own: dict[str, Any] = { + "state": "open", + "head": source_head(head), + "base": {"ref": "main", "repo": {"full_name": "openai/example"}}, + } + measured = { + "schema_version": 1, + "status": "error", + "head_sha": head, + "target_base_sha": "b" * 40, + "error": "synthetic failure", + } + for foreign_head in ( + {**own["head"], "ref": "other"}, + {**own["head"], "repo": {**own["head"]["repo"], "id": 11}}, + {**own["head"], "repo": {**own["head"]["repo"], "full_name": "other/repo"}}, + {**own["head"], "repo": None}, + ): + foreign = {**own, "head": foreign_head} + cases: tuple[list[Any], ...] = ([foreign, run], [own, run, own, [], foreign]) + for states in cases: + with ( + self.subTest(head=foreign_head, reads=len(states)), + mock.patch.object(report, "api", side_effect=states) as api, + ): + self.assertEqual( + report.publish_comment(measured, "openai/example", 3, 123, 1), + "Skipped stale report", + ) + self.assertTrue(all(call.args[0] == "GET" for call in api.call_args_list)) + @unittest.skipUnless(shutil.which("node"), "Node is needed to execute the status-publisher fixture") class StatusPublisherTests(unittest.TestCase): @@ -428,6 +615,7 @@ def publish( policy_changed: bool = False, policy_sha: str | None = None, selected_policy_matches: bool = True, + main_snapshot: str = "", queue_absent: bool = False, queue_pages: list[dict[str, Any]] | None = None, retarget_during_publication: bool = False, @@ -484,7 +672,7 @@ def publish( "number": 3, "user": {"login": "author"}, "state": "open", - "head": {"sha": "c" * 40 if head_changed else head}, + "head": source_head("c" * 40 if head_changed else head), "base": { "sha": "c" * 40 if base_changed else base, "ref": branch, @@ -498,7 +686,9 @@ def publish( else json.dumps( policy.HUMAN_REVIEW_RULES if review_contract is None else review_contract ), + "MAIN_SNAPSHOT": main_snapshot, "SELECTED_BRANCH": branch, + "SELECTED_BOOTSTRAP": "false", "SELECTED_BASE_SHA": base, "SELECTED_HEAD_SHA": head, "SELECTED_POLICY_SHA": (policy_sha or base) @@ -652,7 +842,7 @@ def test_queue_policy_approval_dismissal_after_selection_fails(self) -> None: "number": 7, "user": {"login": "author"}, "state": "open", - "head": {"sha": candidate}, + "head": source_head(candidate), "base": {"ref": "next", "repo": {"full_name": "openai/example"}}, } approved = { @@ -701,7 +891,7 @@ def test_queue_policy_constituent_changes_fail_closed(self) -> None: "number": 7, "user": {"login": "author"}, "state": "open", - "head": {"sha": candidate}, + "head": source_head(candidate), "base": {"ref": "next", "repo": {"full_name": "openai/example"}}, } approved = { @@ -811,6 +1001,22 @@ def test_next_pin_receipt_and_target_freshness(self) -> None: results = self.publish(event_name="merge_group", branch="next", queue_absent=True) self.assertEqual([r["state"] for r in results], ["failure", "failure"]) + def test_main_snapshot_publication_rechecks_live_main(self) -> None: + for event in ("pull_request", "merge_group"): + for moved in (False, True): + with self.subTest(event=event, moved=moved): + writes = self.publish( + event_name=event, + branch="next", + policy_sha="e" * 40, + main_snapshot="e" * 40, + policy_changed=moved, + ) + self.assertEqual( + [w["state"] for w in writes], + ["failure", "failure"] if moved else ["success", "success"], + ) + def test_main_statuses_bind_target_policy_and_queue_membership(self) -> None: for event in ("pull_request", "merge_group"): with self.subTest(event=event): diff --git a/scripts/castiron/test_custom_code_report.py b/scripts/castiron/test_custom_code_report.py index 6c42233e25..a2c9f41909 100644 --- a/scripts/castiron/test_custom_code_report.py +++ b/scripts/castiron/test_custom_code_report.py @@ -18,6 +18,24 @@ class CustomCodeTests(GitTestCase): + def test_large_blob_batches_preserve_every_object_byte(self) -> None: + commands: list[bytes] = [] + expected: dict[str, bytes] = {} + entries: dict[bytes, report.Entry] = {} + for index in range(2048): + content = f"synthetic object {index}\n".encode() + b"x" * 1024 + oid = hashlib.sha1( + f"blob {len(content)}\0".encode() + content, usedforsecurity=False + ).hexdigest() + expected[oid] = content + entries[f"file-{index}".encode()] = report.Entry(b"100644", oid) + commands.extend([b"blob\n", f"data {len(content)}\n".encode(), content, b"\n"]) + report.git(self.repo, "fast-import", "--quiet", input_bytes=b"".join(commands)) + actual = report.read_blobs(self.repo, entries) + self.assertEqual(actual.keys(), expected.keys()) + for oid, content in expected.items(): + self.assertEqual(actual[oid], content) + def test_git_preserves_authentication_without_inherited_repository_routing(self) -> None: environment = { "GIT_CONFIG_GLOBAL": "/ordinary/gitconfig", diff --git a/scripts/castiron/test_custom_code_review_revocation.py b/scripts/castiron/test_custom_code_review_revocation.py index 4baa11512e..90b39773f8 100644 --- a/scripts/castiron/test_custom_code_review_revocation.py +++ b/scripts/castiron/test_custom_code_review_revocation.py @@ -12,7 +12,7 @@ from typing import Any import custom_code_policy as policy -from custom_code_test_support import WORKFLOW, run_publisher, source_run +from custom_code_test_support import WORKFLOW, run_publisher, source_head, source_run @unittest.skipUnless( @@ -25,7 +25,7 @@ def payload(self) -> dict[str, Any]: "number": 3, "user": {"login": "author"}, "state": "open", - "head": {"sha": head}, + "head": source_head(head), "base": {"ref": "next", "sha": base, "repo": {"full_name": "openai/example"}}, } run = { @@ -95,6 +95,39 @@ def test_retries_policy_failure_when_main_advances_before_revocation(self) -> No self.assertEqual({item["sha"] for item in result[:4]}, {"a" * 40, "c" * 40}) self.assertEqual([item["run_id"] for item in result[4:]], [12, 14]) + def test_uses_live_target_when_pull_base_snapshot_lags(self) -> None: + for requires_human in (False, True): + with self.subTest(requires_human=requires_human): + payload = self.payload() + payload["target_sha"] = "e" * 40 + payload["expected_policy_base"] = "e" * 40 + payload["requires_human"] = requires_human + result = self.invoke(payload) + if requires_human: + self.assertEqual([item.get("state") for item in result[:4]], ["pending"] * 4) + self.assertEqual([item["run_id"] for item in result[4:]], [12, 14]) + else: + self.assertEqual(result, []) + + def test_retries_when_live_target_advances_during_evaluation(self) -> None: + for requires_human in (False, True): + with self.subTest(requires_human=requires_human): + payload = self.payload() + payload["target_sha_sequence"] = ["d" * 40, "e" * 40] + payload["target_sha"] = "e" * 40 + payload["expected_policy_bases"] = ["d" * 40, "e" * 40] + payload["requires_human"] = requires_human + result = self.invoke(payload) + if requires_human: + self.assertEqual( + [item["run_id"] for item in result if "run_id" in item], [12, 14] + ) + self.assertTrue( + all(item.get("state") == "pending" for item in result if "state" in item) + ) + else: + self.assertEqual(result, []) + def test_refreshes_executable_policy_when_main_changes_review_requirement(self) -> None: payload = self.payload() payload["main_sha_sequence"] = ["d" * 40, "e" * 40] @@ -421,7 +454,7 @@ def test_empty_review_pr_list_uses_authenticated_fork_discovery(self) -> None: payload["run"].update( { "pull_requests": [], - "head_repository": {"owner": {"login": "contributor"}}, + "head_repository": source_run("a" * 40)["head_repository"], "head_branch": "sdk", } ) @@ -437,7 +470,9 @@ def test_shared_head_revokes_the_run_source_pr(self) -> None: payload = self.payload() payload["run"]["pull_requests"] = [] payload["run"]["head_repository"]["id"] = 101 - payload["current"]["head"].update(ref="sdk", repo={"id": 101}) + payload["current"]["head"].update( + ref="sdk", repo={"id": 101, "full_name": "contributor/example"} + ) payload["associated_pulls"] = [{"number": number} for number in (3, 4, 5)] payload["pulls_by_number"] = { str(number): { @@ -559,7 +594,7 @@ def test_rerun_requires_the_reviewed_pr_not_just_shared_head(self) -> None: fork: dict[str, Any] = { **correct, "pull_requests": [], - "head_repository": {"owner": {"login": "contributor"}}, + "head_repository": source_run("a" * 40)["head_repository"], "head_branch": "sdk", } payload["fallback_pulls"] = [{"number": 3}] @@ -614,6 +649,199 @@ def test_signal_is_unprivileged_and_writers_serialize(self) -> None: handler.count("group: castiron-budget-writes-${{ github.repository_id }}"), 2 ) + def test_direct_head_review_discovery_rejects_shared_commit_from_other_sources( + self, + ) -> None: + for field, value in ( + ("id", 99), + ("full_name", "other/example"), + ("ref", "other-branch"), + ): + with self.subTest(field=field): + payload = self.payload() + payload["run"]["pull_requests"] = [] + head = source_head("a" * 40) + if field == "ref": + head[field] = value + else: + head["repo"][field] = value + other = {**payload["current"], "number": 4, "head": head} + payload["pulls_by_number"] = {"4": other} + for discovery in ("associated_pulls", "fallback_pulls"): + payload["associated_pulls"] = [] + payload[discovery] = [{"number": 4}, {"number": 3}] + result = self.invoke(payload) + self.assertEqual(len(result), 6) + self.assertEqual( + [item["run_id"] for item in result if "run_id" in item], [12, 14] + ) + + def test_foreign_only_associations_retry_revocation_and_reporter_discovery(self) -> None: + for explicit in (False, True): + with self.subTest(explicit=explicit): + payload = self.payload() + payload["run"]["pull_requests"] = [{"number": 4}] if explicit else [] + payload["runs_by_sha"]["a" * 40][0]["pull_requests"] = [] + payload["associated_pulls"] = [{"number": 4}] + payload["fallback_pulls"] = [{"number": 3}] + own = payload["current"] + foreign: dict[str, Any] = { + **own, + "number": 4, + "head": {**own["head"], "ref": "other"}, + } + payload["pulls_by_number"] = {"4": foreign} + payload["review_nodes"] = [{**self.approval(), "state": "DISMISSED"}] + writes = self.invoke(payload) + self.assertEqual( + [w["state"] for w in writes if "state" in w], + [] if explicit else ["pending"] * 4, + ) + self.assertEqual( + [w["run_id"] for w in writes if "run_id" in w], [] if explicit else [12, 14] + ) + # A fallback reached through unrelated commit associations is still branch-only: + # an old synthetic merge must not gain the association-only exemption. + payload["run"]["pull_requests"] = [] + payload["run"]["head_sha"] = payload["context"]["payload"]["workflow_run"]["head_sha"] = ( + "e" * 40 + ) + payload["merge_sha"] = "d" * 40 + payload["merge_commit"] = {"parents": [{"sha": "b" * 40}, {"sha": "a" * 40}]} + payload["pulls_by_number"]["4"]["head"]["sha"] = "f" * 40 + self.assertEqual(self.invoke(payload), []) + + def test_merge_review_filters_foreign_sources_before_branch_fallback(self) -> None: + for field, value in (("ref", "other"), ("id", 99), ("full_name", "other/example")): + for explicit in (False, True): + with self.subTest(field=field, explicit=explicit): + payload = self.payload() + payload["run"]["head_sha"] = "e" * 40 + payload["context"]["payload"]["workflow_run"]["head_sha"] = "e" * 40 + payload["run"]["pull_requests"] = [{"number": 4}] if explicit else [] + payload["associated_pulls"] = [{"number": 4}] + payload["fallback_pulls"] = [{"number": 3}] + payload["merge_sha"] = "e" * 40 + payload["merge_commit"] = {"parents": [{"sha": "b" * 40}, {"sha": "a" * 40}]} + head = source_head("a" * 40) + if field == "ref": + head[field] = value + else: + head["repo"][field] = value + payload["pulls_by_number"] = { + "4": {**payload["current"], "number": 4, "head": head} + } + writes = self.invoke(payload) + self.assertEqual( + [w["state"] for w in writes if "state" in w], + [] if explicit else ["pending"] * 4, + ) + self.assertEqual( + [w["run_id"] for w in writes if "run_id" in w], + [] if explicit else [12, 14], + ) + # Filtering a foreign association must not exempt fallback from freshness. + payload["merge_sha"] = "f" * 40 + self.assertEqual(self.invoke(payload), []) + + def test_review_requires_complete_source_identity(self) -> None: + for overrides in ( + {"head_repository": None}, + {"head_repository": {"id": 10}}, + {"head_repository": {"full_name": "contributor/example"}}, + {"head_branch": None}, + {"head_branch": "refs/pull/3/merge"}, + ): + for merge in (False, True): + with self.subTest(overrides=overrides, merge=merge): + payload = self.payload() + if merge: + payload["run"]["head_sha"] = "e" * 40 + payload["context"]["payload"]["workflow_run"]["head_sha"] = "e" * 40 + payload["merge_commit"] = { + "parents": [{"sha": "b" * 40}, {"sha": "a" * 40}] + } + payload["run"].update(overrides) + self.assertEqual(self.invoke(payload), []) + + def test_merge_sha_signal_requires_exact_live_merge_and_parents(self) -> None: + payload = self.payload() + payload["run"]["head_sha"] = payload["context"]["payload"]["workflow_run"]["head_sha"] = ( + "e" * 40 + ) + payload["merge_sha"] = "e" * 40 + payload["merge_commit"] = {"parents": [{"sha": "b" * 40}, {"sha": "a" * 40}]} + self.assertEqual(len(self.invoke(payload)), 6) + payload["merge_commit"]["parents"].reverse() + self.assertEqual(self.invoke(payload), []) + + def test_historical_merge_revocation_after_base_advance(self) -> None: + for discovery in ("run", "commit", "branch"): + for revoked in ({"state": "DISMISSED"}, {"body": ""}): + with self.subTest(discovery=discovery, revoked=revoked): + payload = self.payload() + payload["review_nodes"] = [{**self.approval(), **revoked}] + payload["run"]["head_sha"] = "e" * 40 + payload["context"]["payload"]["workflow_run"]["head_sha"] = "e" * 40 + payload["current"]["base"]["sha"] = "f" * 40 + payload["comparisons"] = { + f"{'f' * 40}...{'a' * 40}": {"merge_base_commit": {"sha": "b" * 40}} + } + payload["merge_sha"] = "d" * 40 + payload["merge_commit"] = {"parents": [{"sha": "b" * 40}, {"sha": "a" * 40}]} + if discovery != "run": + payload["run"]["pull_requests"] = [] + payload["associated_pulls"] = ( + [{"number": 3}] if discovery == "commit" else [] + ) + payload["fallback_pulls"] = [{"number": 3}] + result = self.invoke(payload) + if discovery == "branch": + self.assertEqual(result, []) + else: + self.assertEqual( + [item.get("state") for item in result[:4]], ["pending"] * 4 + ) + self.assertEqual({item["sha"] for item in result[:4]}, {"a" * 40, "c" * 40}) + self.assertEqual([item["run_id"] for item in result[4:]], [12, 14]) + + def test_rerun_ignores_same_sha_main_from_another_source(self) -> None: + for changes in ( + {"ref": "other"}, + {"repo": {**source_head("a" * 40)["repo"], "id": 11}}, + {"repo": {**source_head("a" * 40)["repo"], "full_name": "other/repo"}}, + {"repo": None}, + ): + with self.subTest(changes=changes): + payload = self.payload() + reporter = payload["runs_by_sha"]["a" * 40][0] + reporter["pull_requests"] = [] + current = payload["current"] + main = { + **current, + "number": 4, + "head": {**current["head"], **changes}, + "base": {**current["base"], "ref": "main"}, + } + payload["pulls_by_number"] = {"4": main} + payload["associated_pulls"] = [{"number": 3}, {"number": 4}] + result = self.invoke(payload) + self.assertEqual([item["run_id"] for item in result if "run_id" in item], [12, 14]) + + def test_rerun_rejects_missing_reporter_source_identity(self) -> None: + for changes in ( + {"head_repository": None}, + {"head_repository": {"full_name": "contributor/example"}}, + {"head_repository": {"id": 10}}, + {"head_branch": ""}, + ): + with self.subTest(changes=changes): + payload = self.payload() + payload["runs_by_sha"]["a" * 40][0].update(changes) + result = self.invoke(payload) + self.assertEqual([item["run_id"] for item in result if "run_id" in item], [14]) + self.assertTrue(all(item["state"] == "pending" for item in result[:4])) + if __name__ == "__main__": unittest.main() diff --git a/scripts/castiron/test_custom_code_status_publication.py b/scripts/castiron/test_custom_code_status_publication.py new file mode 100644 index 0000000000..7fe6edf2f5 --- /dev/null +++ b/scripts/castiron/test_custom_code_status_publication.py @@ -0,0 +1,694 @@ +# File generated from our OpenAPI spec by Castiron. See CONTRIBUTING.md for details. +"""Execute the trusted workflow against offline GitHub state.""" + +from __future__ import annotations + +import json +import shutil +import unittest +from typing import Any + +import custom_code_policy as policy +from custom_code_test_support import run_publisher, source_head, source_run + + +@unittest.skipUnless(shutil.which("node"), "Node is needed to execute the status-publisher fixture") +class StatusPublisherTests(unittest.TestCase): + def publish( + self, + *, + event_name: str = "pull_request", + head_changed: bool = False, + base_changed: bool = False, + no_result: bool = False, + failed_budget: bool = False, + fallback_pulls: list[dict[str, int]] | None = None, + associated_pulls: list[dict[str, int]] | None = None, + run_overrides: dict[str, Any] | None = None, + previous_statuses: list[dict[str, Any]] | None = None, + branch: str = "main", + evaluated_branch: str | None = None, + policy_changed: bool = False, + bootstrap: str = "false", + policy_sha: str | None = None, + selected_policy_matches: bool = True, + queue_absent: bool = False, + queue_pages: list[dict[str, Any]] | None = None, + retarget_during_publication: bool = False, + source_changes_during_publication: dict[str, Any] | None = None, + same_head_next_pr: bool = False, + policy_reviews: list[dict[str, Any]] | None = None, + reviews: list[dict[str, Any]] | None = None, + review_nodes: list[dict[str, Any]] | None = None, + review_pages: list[dict[str, Any]] | None = None, + policy_pull: dict[str, Any] | None = None, + missing_review_contract: bool = False, + review_contract: dict[str, Any] | None = None, + ) -> list[dict[str, Any]]: + base, head = "a" * 40, "b" * 40 + run = source_run(head, event_name) + if event_name == "merge_group": + run["head_branch"] = f"gh-readonly-queue/{branch}/pr-7-example" + payload: dict[str, Any] = { + "reviews": reviews or [], + "review_nodes": review_nodes + if review_nodes is not None + else [ + { + "fullDatabaseId": str(review["id"]), + "state": review["state"], + "body": review.get("body"), + "commit": {"oid": review.get("commit_id")}, + "author": { + "login": review["user"]["login"], + "__typename": review["user"]["type"], + }, + "authorAssociation": review.get("author_association"), + } + for review in reviews or [] + ], + "review_pages": review_pages, + "main_sha": "d" * 40 + if policy_changed + else (policy_sha or ("c" * 40 if branch == "main" and base_changed else base)), + "queue_absent": queue_absent, + "queue_pages": queue_pages, + "fallback_pulls": fallback_pulls, + "associated_pulls": associated_pulls, + "previous_statuses": previous_statuses or [], + "context": { + "eventName": "workflow_run", + "repo": {"owner": "openai", "repo": "example"}, + "serverUrl": "https://github.com", + "runId": 123, + "payload": { + "workflow_run": run, + }, + }, + "run": {**run, **(run_overrides or {})}, + "current": { + "number": 3, + "user": {"login": "author"}, + "state": "open", + "head": source_head("c" * 40 if head_changed else head), + "base": { + "sha": "c" * 40 if base_changed else base, + "ref": branch, + "repo": {"full_name": "openai/example"}, + }, + }, + "env": { + "POLICY_REVIEWS": json.dumps(policy_reviews or []), + "HUMAN_REVIEW_RULES": "" + if missing_review_contract + else json.dumps( + policy.HUMAN_REVIEW_RULES if review_contract is None else review_contract + ), + "SELECTED_BRANCH": branch, + "SELECTED_BOOTSTRAP": bootstrap, + "SELECTED_BASE_SHA": base, + "SELECTED_HEAD_SHA": head, + "SELECTED_POLICY_SHA": (policy_sha or base) + if selected_policy_matches + else "f" * 40, + "TARGET_BRANCH": "" if no_result else (evaluated_branch or branch), + "POLICY_SHA": "" if no_result else (policy_sha or base), + "BASE_SHA": "" if no_result else base, + "HEAD_SHA": "" if no_result else head, + "ISOLATION_RESULT": "success", + "BUDGET_RESULT": "failure" if failed_budget else "success", + "PUBLISH_ATTEMPT": "1", + }, + } + if policy_pull: + payload["pulls_by_number"] = {policy_pull["number"]: policy_pull} + if same_head_next_pr: + current = payload["current"] + payload["pulls_by_number"] = { + 3: current, + 4: {**current, "number": 4, "base": {**current["base"], "ref": "next"}}, + } + if retarget_during_publication: + current = payload["current"] + payload["current_sequence"] = [ + current, + {**current, "base": {**current["base"], "ref": "other"}}, + ] + if source_changes_during_publication is not None: + current = payload["current"] + payload["current_sequence"] = [ + current, + {**current, "head": {**current["head"], **source_changes_during_publication}}, + ] + return run_publisher("Publish exact-head statuses after checking freshness", payload) + + def test_live_human_review_predicate_matches_selector(self) -> None: + head = "b" * 40 + approved = { + "id": 10, + "state": "APPROVED", + "commit_id": head, + "user": {"login": "human", "type": "User"}, + "author_association": "MEMBER", + "body": policy.HUMAN_REVIEW_RULES["attestation_prefix"] + head, + } + cases = [ + [approved], + [], + [{**approved, "state": "DISMISSED"}], + [approved, {**approved, "id": 11, "state": "CHANGES_REQUESTED"}], + [approved, {**approved, "id": 11, "state": "COMMENTED"}], + [{**approved, "commit_id": "f" * 40}], + [{**approved, "user": {"login": "author", "type": "User"}}], + [{**approved, "user": {"login": "bot", "type": "Bot"}}], + [{**approved, "author_association": "NONE"}], + ] + for body in [ + "", + "Looks good to me", + policy.HUMAN_REVIEW_RULES["attestation_prefix"] + "f" * 40, + "> " + policy.HUMAN_REVIEW_RULES["attestation_prefix"] + head, + "", + "Automated source review.", + "Source-only review; I did not execute candidate code or workflows.", + "This does not substitute for human approval.", + "This is not a human approving review.", + ]: + cases.append([{**approved, "body": body}]) + for reviews in cases: + with self.subTest(reviews=reviews): + expected = ( + "success" if policy.has_human_approval(reviews, "author", head) else "failure" + ) + writes = self.publish( + branch="next", + policy_reviews=[{"number": 3, "head_sha": head}], + reviews=reviews, + ) + self.assertEqual([w["state"] for w in writes], [expected, expected]) + + def test_final_publisher_preserves_review_identity_precision(self) -> None: + head = "b" * 40 + approval = { + "id": 9007199254740992, + "state": "APPROVED", + "commit_id": head, + "user": {"login": "human", "type": "User"}, + "author_association": "MEMBER", + "body": policy.HUMAN_REVIEW_RULES["attestation_prefix"] + head, + } + replacement = {**approval, "id": 9007199254740993, "state": "COMMENTED"} + for reviews in ([approval, replacement], [replacement, approval]): + with self.subTest(reviews=reviews): + writes = self.publish( + branch="next", + policy_reviews=[{"number": 3, "head_sha": head}], + reviews=reviews, + ) + self.assertEqual([w["state"] for w in writes], ["failure"] * 2) + writes = self.publish( + branch="next", + policy_reviews=[{"number": 3, "head_sha": head}], + reviews=[{**replacement, "state": "APPROVED"}, {**approval, "state": "DISMISSED"}], + ) + self.assertEqual([w["state"] for w in writes], ["success"] * 2) + + def test_final_publisher_review_pages_and_malformed_identity(self) -> None: + head = "b" * 40 + approval = { + "fullDatabaseId": "9007199254740992", + "state": "APPROVED", + "commit": {"oid": head}, + "author": {"login": "human", "__typename": "User"}, + "authorAssociation": "MEMBER", + "body": policy.HUMAN_REVIEW_RULES["attestation_prefix"] + head, + } + + def publish(**kwargs: Any) -> list[str]: + return [ + w["state"] + for w in self.publish( + branch="next", + policy_reviews=[{"number": 3, "head_sha": head}], + **kwargs, + ) + ] + + self.assertEqual(publish(review_nodes=[approval]), ["success"] * 2) + for identity in (None, "", "1.5", "-1", "0", 9007199254740992): + with self.subTest(identity=identity): + self.assertEqual( + publish(review_nodes=[{**approval, "fullDatabaseId": identity}]), + ["failure"] * 2, + ) + pages: list[dict[str, Any]] = [ + { + "expected_cursor": None, + "nodes": [approval], + "pageInfo": {"hasNextPage": True, "endCursor": "page2"}, + }, + { + "expected_cursor": "page2", + "nodes": [{**approval, "fullDatabaseId": "9007199254740993", "state": "DISMISSED"}], + "pageInfo": {"hasNextPage": False, "endCursor": None}, + }, + ] + self.assertEqual(publish(review_pages=pages), ["failure"] * 2) + self.assertEqual( + publish( + review_pages=[{**pages[0], "pageInfo": {"hasNextPage": True, "endCursor": None}}] + ), + ["failure"] * 2, + ) + + def test_bootstrap_rechecks_authenticated_main(self) -> None: + for event in ("pull_request", "merge_group"): + for bootstrap, changed, expected in ( + ("true", False, "success"), + ("true", True, "failure"), + ("false", True, "success"), + ("", False, "failure"), + ("invalid", False, "failure"), + ): + with self.subTest(event=event, bootstrap=bootstrap, changed=changed): + writes = self.publish( + event_name=event, + branch="next", + bootstrap=bootstrap, + policy_changed=changed, + ) + self.assertEqual([item["state"] for item in writes], [expected] * 2) + + def test_freshness_is_rechecked_after_review_pagination(self) -> None: + head = "b" * 40 + approval = { + "fullDatabaseId": "1", + "state": "APPROVED", + "body": policy.HUMAN_REVIEW_RULES["attestation_prefix"] + head, + "author": {"login": "human", "__typename": "User"}, + "authorAssociation": "MEMBER", + "commit": {"oid": head}, + } + current: dict[str, Any] = { + "number": 3, + "user": {"login": "author"}, + "state": "open", + "head": source_head(head), + "base": {"ref": "next", "sha": "a" * 40, "repo": {"full_name": "openai/example"}}, + } + advanced = {**current, "base": {**current["base"], "sha": "c" * 40}} + changed_source = {**current, "head": {**current["head"], "ref": "other-source"}} + for event, mutation, expected in ( + ("pull_request", {}, ["success"] * 2), + ("merge_group", {}, ["success"] * 2), + ("pull_request", {"main_sha": "d" * 40}, ["failure"] * 2), + ("merge_group", {"main_sha": "d" * 40}, ["failure"] * 2), + ("pull_request", {"current": advanced}, ["failure"] * 2), + ("merge_group", {"current": advanced}, ["failure"] * 2), + ("merge_group", {"queue_absent": True}, ["failure"] * 2), + ("pull_request", {"current": changed_source}, []), + ): + with self.subTest(event=event, mutation=mutation): + result = self.publish( + event_name=event, + branch="next", + bootstrap="true", + policy_reviews=[{"number": 3, "head_sha": head}], + review_pages=[ + { + "expected_cursor": None, + "nodes": [], + "pageInfo": {"hasNextPage": True, "endCursor": "last"}, + }, + { + "expected_cursor": "last", + "nodes": [approval], + "pageInfo": {"hasNextPage": False, "endCursor": None}, + "after_read": mutation, + }, + ], + ) + self.assertEqual([item["state"] for item in result], expected) + + def test_queue_policy_approval_dismissal_after_selection_fails(self) -> None: + candidate = "e" * 40 + pull: dict[str, Any] = { + "number": 7, + "user": {"login": "author"}, + "state": "open", + "head": {"sha": candidate}, + "base": {"ref": "next", "repo": {"full_name": "openai/example"}}, + } + approved = { + "id": 1, + "state": "APPROVED", + "commit_id": candidate, + "user": {"login": "human", "type": "User"}, + "author_association": "MEMBER", + "body": policy.HUMAN_REVIEW_RULES["attestation_prefix"] + candidate, + } + for state in ("APPROVED", "DISMISSED", "CHANGES_REQUESTED"): + with self.subTest(state=state): + writes = self.publish( + event_name="merge_group", + branch="next", + policy_pull=pull, + policy_reviews=[{"number": 7, "head_sha": candidate}], + reviews=[{**approved, "state": state}], + ) + self.assertEqual( + [w["state"] for w in writes], + ["success" if state == "APPROVED" else "failure"] * 2, + ) + # Ordinary automated approvals may keep the queue live, but cannot replace + # the independently required human policy approval. + writes = self.publish( + event_name="merge_group", + branch="next", + policy_pull=pull, + policy_reviews=[{"number": 7, "head_sha": candidate}], + reviews=[ + {**approved, "state": "DISMISSED"}, + { + **approved, + "id": 2, + "user": {"login": "automated", "type": "User"}, + "body": "", + }, + ], + ) + self.assertEqual([w["state"] for w in writes], ["failure", "failure"]) + + def test_queue_policy_constituent_changes_fail_closed(self) -> None: + candidate = "e" * 40 + pull: dict[str, Any] = { + "number": 7, + "user": {"login": "author"}, + "state": "open", + "head": {"sha": candidate}, + "base": {"ref": "next", "repo": {"full_name": "openai/example"}}, + } + approved = { + "id": 1, + "state": "APPROVED", + "commit_id": candidate, + "user": {"login": "human", "type": "User"}, + "author_association": "MEMBER", + "body": policy.HUMAN_REVIEW_RULES["attestation_prefix"] + candidate, + } + for changed in [ + {**pull, "state": "closed"}, + {**pull, "head": {"sha": "f" * 40}}, + {**pull, "base": {**pull["base"], "ref": "main"}}, + ]: + with self.subTest(changed=changed): + writes = self.publish( + event_name="merge_group", + branch="next", + policy_pull=changed, + policy_reviews=[{"number": 7, "head_sha": candidate}], + reviews=[approved], + ) + self.assertEqual([w["state"] for w in writes], ["failure", "failure"]) + + def test_missing_policy_review_contract_fails_closed(self) -> None: + self.assertEqual( + [w["state"] for w in self.publish(branch="next", missing_review_contract=True)], + ["failure", "failure"], + ) + + def test_legacy_review_contract_fails_closed_only_on_next(self) -> None: + legacy = { + key: value + for key, value in policy.HUMAN_REVIEW_RULES.items() + if key != "attestation_prefix" + } + legacy["version"] = 1 + for branch, expected in (("next", "failure"), ("main", "success")): + with self.subTest(branch=branch): + self.assertEqual( + [w["state"] for w in self.publish(branch=branch, review_contract=legacy)], + [expected, expected], + ) + + def test_statuses_recheck_source_identity_immediately_before_writing(self) -> None: + source = source_head("b" * 40) + self.assertEqual(len(self.publish(source_changes_during_publication={})), 2) + for changes in ( + {"ref": "other"}, + {"repo": {**source["repo"], "id": 11}}, + {"repo": {**source["repo"], "full_name": "other/example"}}, + {"repo": None}, + ): + with self.subTest(changes=changes): + self.assertEqual(self.publish(source_changes_during_publication=changes), []) + + def test_statuses_reject_a_run_from_a_different_source(self) -> None: + for source in ( + {"head_branch": "other"}, + {"head_repository": {"id": 11, "full_name": "other/repo"}}, + {"head_repository": None}, + ): + with self.subTest(source=source): + self.assertEqual(self.publish(run_overrides=source), []) + + def test_statuses_attach_to_candidate_not_main(self) -> None: + for event in ("pull_request", "merge_group"): + with self.subTest(event=event): + results = self.publish(event_name=event) + self.assertEqual(len(results), 2) + self.assertTrue( + all(r["sha"] == "b" * 40 and r["state"] == "success" for r in results) + ) + + def test_next_cannot_overwrite_main_results(self) -> None: + for main_event in ("pull_request", "merge_group"): + main = self.publish(event_name=main_event, failed_budget=True) + self.assertEqual([r["state"] for r in main], ["success", "failure"]) + for event in ("pull_request", "merge_group"): + with self.subTest(main_event=main_event, event=event): + # Both evaluations use the same commit. A next success + # must not replace the main target's required failure. + result = self.publish(event_name=event, branch="next", previous_statuses=main) + self.assertEqual([r["state"] for r in result], ["success", "success"]) + self.assertTrue( + {r["context"] for r in result}.isdisjoint(r["context"] for r in main) + ) + + def test_foreign_only_associations_retry_source_branch(self) -> None: + foreign = { + "number": 4, + "state": "open", + "head": {**source_head("b" * 40), "ref": "other"}, + "base": {"ref": "main", "repo": {"full_name": "openai/example"}}, + } + for explicit in (False, True): + with self.subTest(explicit=explicit): + writes = self.publish( + run_overrides={"pull_requests": [{"number": 4}] if explicit else []}, + associated_pulls=[{"number": 4}], + fallback_pulls=[{"number": 3}], + policy_pull=foreign, + ) + self.assertEqual( + [w["state"] for w in writes], [] if explicit else ["success", "success"] + ) + + def test_same_head_next_pr_does_not_block_required_main_statuses(self) -> None: + for numbers in ([3, 4], [4, 3]): + with self.subTest(numbers=numbers): + results = self.publish( + run_overrides={"pull_requests": []}, + fallback_pulls=[{"number": number} for number in numbers], + same_head_next_pr=True, + failed_budget=True, + ) + self.assertEqual([r["state"] for r in results], ["success", "failure"]) + self.assertTrue(all(r["sha"] == "b" * 40 for r in results)) + next_results = self.publish( + branch="next", + run_overrides={"pull_requests": [{"number": 4}]}, + same_head_next_pr=True, + ) + self.assertEqual([r["state"] for r in next_results], ["success", "success"]) + self.assertTrue(all(r["context"].startswith("Castiron next /") for r in next_results)) + + def test_next_queue_approved_pin_survives_main_advancing(self) -> None: + for changed in (False, True): + with self.subTest(policy_changed=changed): + results = self.publish( + event_name="merge_group", branch="next", policy_changed=changed + ) + self.assertEqual([r["state"] for r in results], ["success", "success"]) + + def test_next_pin_receipt_and_target_freshness(self) -> None: + for event in ("pull_request", "merge_group"): + cases: tuple[dict[str, Any], ...] = ( + {"selected_policy_matches": False}, + {"no_result": True}, + {"base_changed": True}, + ) + for kwargs in cases: + with self.subTest(event=event, kwargs=kwargs): + results = self.publish(event_name=event, branch="next", **kwargs) + self.assertEqual([r["state"] for r in results], ["failure", "failure"]) + results = self.publish(event_name=event, branch="next", policy_sha="e" * 40) + self.assertEqual([r["state"] for r in results], ["success", "success"]) + results = self.publish(event_name="merge_group", branch="next", queue_absent=True) + self.assertEqual([r["state"] for r in results], ["failure", "failure"]) + + def test_main_statuses_bind_target_policy_and_queue_membership(self) -> None: + for event in ("pull_request", "merge_group"): + with self.subTest(event=event): + stale = self.publish(event_name=event, evaluated_branch="next") + self.assertEqual([r["state"] for r in stale], ["failure", "failure"]) + for policy_changed, queue_absent in ((True, False), (False, True)): + with self.subTest(policy_changed=policy_changed, queue_absent=queue_absent): + result = self.publish( + event_name="merge_group", + policy_changed=policy_changed, + queue_absent=queue_absent, + ) + self.assertEqual([r["state"] for r in result], ["failure", "failure"]) + self.assertEqual(self.publish(retarget_during_publication=True), []) + + def test_next_fork_and_unsupported_targets(self) -> None: + result = self.publish( + branch="next", run_overrides={"pull_requests": []}, fallback_pulls=[{"number": 3}] + ) + self.assertEqual([r["state"] for r in result], ["success", "success"]) + for event in ("pull_request", "merge_group"): + for branch in ("Next", "Main", "release", "next-other"): + with self.subTest(event=event, branch=branch): + self.assertEqual(self.publish(event_name=event, branch=branch), []) + + def test_main_queue_membership_paginates(self) -> None: + pages: list[dict[str, Any]] = [ + { + "expected_cursor": None, + "nodes": [], + "pageInfo": {"hasNextPage": True, "endCursor": "page2"}, + }, + { + "expected_cursor": "page2", + "nodes": [{"headCommit": {"oid": "b" * 40}}], + "pageInfo": {"hasNextPage": False, "endCursor": None}, + }, + ] + result = self.publish(event_name="merge_group", queue_pages=pages) + self.assertEqual([r["state"] for r in result], ["success", "success"]) + + def test_fork_statuses_with_no_commit_association(self) -> None: + options: dict[str, Any] = { + "run_overrides": {"pull_requests": []}, + "fallback_pulls": [{"number": 3}], + } + results = self.publish(**options) + self.assertEqual(len(results), 2) + self.assertTrue(all(r["sha"] == "b" * 40 and r["state"] == "success" for r in results)) + self.assertEqual(self.publish(**options, head_changed=True), []) + self.assertTrue( + all(r["state"] == "failure" for r in self.publish(**options, no_result=True)) + ) + self.assertEqual(self.publish(run_overrides={"pull_requests": []}, fallback_pulls=[]), []) + self.assertEqual( + self.publish( + run_overrides={"pull_requests": []}, + fallback_pulls=[{"number": 3}, {"number": 4}], + ), + [], + ) + + def test_stale_pr_head_is_not_published(self) -> None: + self.assertEqual(self.publish(head_changed=True), []) + + def test_pr_snapshot_survives_main_advancing(self) -> None: + results = self.publish(base_changed=True) + self.assertEqual(len(results), 2) + self.assertTrue(all(r["state"] == "success" for r in results)) + self.assertTrue(all("a" * 12 in r["description"] for r in results)) + + def test_queue_still_rejects_main_advancing(self) -> None: + results = self.publish(event_name="merge_group", base_changed=True) + self.assertEqual(len(results), 2) + self.assertTrue(all(r["state"] == "failure" for r in results)) + + def test_missing_evaluation_publishes_actionable_failures(self) -> None: + for event in ("pull_request", "merge_group"): + results = self.publish(event_name=event, no_result=True, base_changed=True) + self.assertEqual(len(results), 2) + for result in results: + self.assertEqual(result["state"], "failure") + self.assertIn("inspect the trusted run", result["description"]) + self.assertTrue(result["target_url"].endswith("/actions/runs/123")) + + def test_main_rejects_mismatched_selector_receipt(self) -> None: + results = self.publish(selected_policy_matches=False) + self.assertTrue(results) + self.assertTrue(all(result["state"] != "success" for result in results)) + + def test_lookalike_workflow_cannot_publish(self) -> None: + self.assertEqual( + self.publish( + run_overrides={ + "path": ".github/workflows/castiron-custom-code.yml@evil.yml", + "workflow_id": 789, + } + ), + [], + ) + + def test_superseded_or_wrong_source_run_cannot_publish(self) -> None: + for overrides in ( + {"run_attempt": 2}, + {"head_sha": "c" * 40}, + {"event": "push"}, + {"path": "other.yml"}, + ): + with self.subTest(overrides=overrides): + self.assertEqual(self.publish(run_overrides=overrides), []) + + def test_independent_check_failures_are_preserved(self) -> None: + results = self.publish(failed_budget=True) + self.assertEqual([r["state"] for r in results], ["success", "failure"]) + + def test_older_evaluation_cannot_overwrite_newer_failure(self) -> None: + for order in ("124:1:125:1", "123:2:125:1", "123:1:125:1", "123:1:123:2"): + with self.subTest(order=order): + previous = { + "context": "Castiron / custom-code budget", + "creator": {"login": "github-actions[bot]"}, + "state": "failure", + "description": f"Failed against newer main. [evaluation {order}]", + "target_url": f"https://github.com/openai/example/actions/runs/{order.split(':')[2]}", + } + self.assertEqual(self.publish(previous_statuses=[previous]), []) + + def test_publication_guard_allows_current_retry_and_ignores_unrelated_statuses( + self, + ) -> None: + previous = { + "context": "Castiron / custom-code budget", + "creator": {"login": "github-actions[bot]"}, + "description": "Failed. [evaluation 123:1:123:1]", + "target_url": "https://github.com/openai/example/actions/runs/123", + } + for overrides in ( + {}, # A partial publication can retry the same evaluation. + { + "description": "Failed. [evaluation 122:9:999:9]", + "target_url": "https://github.com/openai/example/actions/runs/999", + }, + {"description": "Legacy status without evaluation marker"}, + {"context": "Other check", "description": "[evaluation 999:9:999:9]"}, + {"creator": {"login": "someone"}, "description": "[evaluation 999:9:999:9]"}, + { + "description": "[evaluation 999:9:999:9]", + "target_url": "https://example.com/999", + }, + ): + with self.subTest(overrides=overrides): + results = self.publish(previous_statuses=[{**previous, **overrides}]) + self.assertEqual(len(results), 2) + self.assertTrue(all(r["state"] == "success" for r in results)) + self.assertTrue(all(len(r["description"]) <= 140 for r in results)) diff --git a/scripts/castiron/test_custom_code_strict.py b/scripts/castiron/test_custom_code_strict.py new file mode 100644 index 0000000000..41acb56f9d --- /dev/null +++ b/scripts/castiron/test_custom_code_strict.py @@ -0,0 +1,731 @@ +# File generated from our OpenAPI spec by Castiron. See CONTRIBUTING.md for details. +# Regression tests for strict generated-file ownership. +from __future__ import annotations + +import hashlib +import json +import subprocess +import sys +import unittest +from pathlib import Path +from typing import Any, cast +from unittest import mock + +import custom_code_budget as budget +import custom_code_report as report +from custom_code_test_support import GitTestCase + + +class StrictTests(GitTestCase): + workflow = ".github/workflows/castiron-custom-code.yml" + + def setUp(self) -> None: # pyright: ignore[reportImplicitOverride] + super().setUp() + self.write("generated.py", "generated\n") + for path in budget.GENERATED_GITHUB_PATHS: + self.write(path.decode(), "workflow\n") + self.inventory(budget.GENERATED_GITHUB_PATHS) + self.manifest() + self.generated, _ = self.baseline() + self.write( + budget.POLICY, + json.dumps( + {"schema_version": 2, "generated_files": "strict", "max_custom_patch_lines": 0} + ), + ) + self.base = self.commit("enable strict ownership") + + def inventory(self, paths: frozenset[bytes]) -> None: + # Checkpoint code is data. A verifier must never execute this statement. + self.write( + "scripts/castiron/custom_code_budget.py", + "raise AssertionError('checkpoint code must not execute')\n" + f"GENERATED_GITHUB_PATHS = frozenset({set(paths)!r})\n", + ) + + def manifest(self, extra: tuple[str, ...] = ()) -> None: + files = { + p: { + "mode": "100644", + "sha256": hashlib.sha256((self.repo / p).read_bytes()).hexdigest(), + } + for p in ( + "generated.py", + *(path.decode() for path in budget.GENERATED_GITHUB_PATHS), + "scripts/castiron/custom_code_budget.py", + *extra, + ) + } + self.write(budget.MANIFEST.decode(), json.dumps({"schema_version": 1, "files": files})) + + def evaluate(self) -> dict[str, Any]: + return budget.evaluate(self.repo, self.base, self.commit(), public=False)[0] + + def test_handwritten_change_passes(self) -> None: + self.write("handwritten.py", "custom behavior\n") + self.assertEqual(self.evaluate()["checks"]["budget"]["state"], "success") + + def test_strict_main_checks_migration_target_and_candidate(self) -> None: + self.git("checkout", "-q", "-b", "next", self.base) + self.write(budget.POLICY, '{"schema_version":1,"max_custom_patch_lines":1000}') + target = self.commit("next still uses migration policy") + self.write(self.workflow, "manual workflow edit\n") + head = self.commit() + for pull_heads in (None, [head]): + with self.subTest(pull_heads=pull_heads): + result, _ = budget.evaluate( + self.repo, + target, + head, + public=False, + policy_sha=self.base, + pull_heads=pull_heads, + ) + self.assertEqual(result["total"], 0) + self.assertEqual(result["checks"]["isolation"]["state"], "success") + self.assertEqual(result["checks"]["budget"]["state"], "failure") + self.assertIn(self.workflow, result["strict_violations"][0]) + + def test_exact_main_snapshot_bootstrap_verifies_real_checkpoints(self) -> None: + repository = "openai/example" + self.git("remote", "add", "origin", f"https://github.com/{repository}.git") + + def public_snapshot(generated: str, parent: str | None) -> str: + stats = report.read_stats(self.repo, self.git("rev-parse", "HEAD")) + return report.create_public_snapshot( + self.repo, + self.git("rev-parse", f"{generated}^{{tree}}"), + stats["generation_id"], + report.hash_codegen_commit(self.repo, generated), + "codegen/test", + parent, + ) + + old_public = public_snapshot(self.generated, None) + stats_path = self.repo / ".castiron.stats.yml" + stats_path.write_text(stats_path.read_text() + f"public_codegen_sha: {old_public}\n") + self.write(budget.POLICY, '{"schema_version":1,"max_custom_patch_lines":4000}') + self.write("generated.py", "legacy custom edit\n") + legacy = self.commit("legacy unpinned next") + self.write("generated.py", "generated\n") + checker = self.repo / "scripts/castiron/custom_code_budget.py" + checker.write_text(checker.read_text() + "NEXT_POLICY_VERSION = 1\n") + self.manifest() + # Generated checkpoints exclude integration metadata and repository policy. + stats_path.unlink() + (self.repo / budget.POLICY).unlink() + generated, _ = self.baseline() + current_public = public_snapshot(generated, old_public) + self.git("update-ref", "refs/remotes/origin/codegen/test", current_public) + stats_path.write_text(stats_path.read_text() + f"public_codegen_sha: {current_public}\n") + strict = {"schema_version": 2, "max_custom_patch_lines": 0, "generated_files": "strict"} + self.write(budget.POLICY, json.dumps(strict)) + main = self.commit("verified strict main") + self.git("update-ref", "refs/remotes/origin/main", main) + self.git("update-ref", "refs/remotes/origin/next", legacy) + real_git = budget.report.git + + def offline_git(repo: Path, *args: str, **kwargs: Any) -> bytes: + # Only network transport is mocked; refs, trees, metadata and verification are real Git. + if args[0] == "fetch": + for spec in args: + if spec.startswith("+refs/heads/"): + source, destination = spec[1:].split(":", 1) + revision = ( + real_git( + repo, + "rev-parse", + source.replace("refs/heads/", "refs/remotes/origin/", 1), + ) + .decode() + .strip() + ) + real_git(repo, "update-ref", destination, revision) + return b"" + return cast(bytes, real_git(repo, *args, **kwargs)) + + def public_api(path: str) -> dict[str, Any]: + if path == f"repos/{repository}": + return {"private": False, "default_branch": "main", "full_name": repository} + return {"object": {"sha": main if path.endswith("/main") else legacy}} + + original_index = self.git("write-tree") + with ( + mock.patch.object(budget.report, "git", side_effect=offline_git), + mock.patch.object(budget, "read_public_api", side_effect=public_api), + ): + prepared = budget.prepare_next_baseline(self.repo, repository, "codex/adopt-main") + candidate = prepared["head_sha"] + self.assertEqual(self.git("rev-parse", "refs/heads/codex/adopt-main"), candidate) + self.assertEqual(self.git("write-tree"), original_index) + self.assertEqual(self.git("rev-parse", "HEAD"), main) + self.assertEqual( + self.git("show", f"{candidate}:.castiron.stats.yml"), + self.git("show", f"{main}:.castiron.stats.yml"), + ) + refs_read = 0 + + def moving_api(path: str) -> dict[str, Any]: + nonlocal refs_read + if path.endswith("/main"): + refs_read += 1 + if refs_read > 1: + return {"object": {"sha": legacy}} + return public_api(path) + + with ( + mock.patch.object(budget.report, "git", side_effect=offline_git), + mock.patch.object(budget, "read_public_api", side_effect=moving_api), + self.assertRaisesRegex(ValueError, "refs changed"), + ): + budget.prepare_next_baseline(self.repo, repository, "codex/stale-main") + self.assertEqual(self.git("for-each-ref", "refs/heads/codex/stale-main"), "") + self.git("checkout", "--detach", "-q", candidate) + wrapper = self.git( + "commit-tree", + f"{candidate}^{{tree}}", + "-p", + legacy, + "-p", + candidate, + "-m", + "verified queue wrapper", + ) + for heads in (None, [candidate]): + with self.subTest(queue=heads is not None): + result, _ = budget.evaluate( + self.repo, + legacy, + candidate if heads is None else wrapper, + public=True, + target_branch="next", + policy_sha=main, + main_snapshot=main, + repository=repository, + pull_heads=heads, + ) + self.assertEqual(result["total"], 0) + self.assertTrue( + all(c["state"] == "success" for c in result["checks"].values()), result + ) + # Exercise the real hosted evaluator with independent, initially empty + # object stores. The selector's origin/main ref is not shared with them. + from test_custom_code_github import GitHubAPI + + remote = self.repo / "bootstrap-remote.git" + self.git("clone", "--bare", str(self.repo), str(remote)) + real_git(remote, "update-ref", "refs/heads/main", main) + real_git(remote, "update-ref", "refs/heads/codegen/test", current_public) + real_git(remote, "update-ref", "refs/heads/queue", wrapper) + named_fetches: list[Path] = [] + + def transport(repo: Path, *args: str, **kwargs: Any) -> bytes: + if args[0] == "fetch": + self.assertIn("origin", args) + if "+refs/heads/main:refs/remotes/origin/main" in args: + named_fetches.append(repo) + args = tuple(str(remote) if arg == "origin" else arg for arg in args) + self.assertNotIn("checkout", args) + return cast(bytes, real_git(repo, *args, **kwargs)) + + for mode in ("pr", "reused-pr", "queue", "moved-main"): + with self.subTest(object_store=mode): + queue = mode in ("queue", "moved-main") + revision = wrapper if queue else candidate + api = GitHubAPI(main, revision, "merge_group" if queue else "pull_request") + api.next = legacy + api.pull["base"]["ref"] = "next" + if queue: + api.run["head_branch"] = "gh-readonly-queue/next/pr-3-test" + objects = self.repo / f"bootstrap-{mode}.git" + out = self.repo / f"bootstrap-{mode}-report" + receipt = { + "schema_version": 1, + "target_branch": "next", + "base_sha": legacy, + "head_sha": revision, + "policy_sha": main, + "bootstrap": True, + "main_snapshot": main, + } + with ( + mock.patch.object(report, "git", side_effect=transport), + mock.patch.object(budget.report, "git", side_effect=transport), + mock.patch.object(report, "api", side_effect=api), + mock.patch.object(budget.report, "api", side_effect=api), + mock.patch.object( + budget, "queued_entries", return_value=[(candidate, wrapper)] + ), + ): + if mode == "reused-pr": + report.trusted_report(objects, repository, 123, 1, out, base=main) + self.assertEqual( + real_git(objects, "for-each-ref", "refs/remotes/origin/main"), b"" + ) + if mode == "moved-main": + # API receipt remains captured at main while Git's named + # ref changes before fetch: the evaluator must fail closed. + real_git(remote, "update-ref", "refs/heads/main", legacy) + with self.assertRaisesRegex(ValueError, "authenticated main changed"): + budget.github_evaluate( + objects, + repository, + {"repository": {"full_name": repository}, "workflow_run": api.run}, + main, + selection=receipt, + ) + else: + result, _ = budget.github_evaluate( + objects, + repository, + {"repository": {"full_name": repository}, "workflow_run": api.run}, + main, + out if mode == "reused-pr" else None, + selection=receipt, + ) + self.assertTrue( + all(c["state"] == "success" for c in result["checks"].values()), result + ) + self.assertEqual(result["total"], 0) + self.assertEqual( + real_git(objects, "rev-parse", "refs/remotes/origin/main") + .decode() + .strip(), + main, + ) + self.assertIn(objects, named_fetches) + self.assertFalse((objects / "scripts").exists()) + self.write("generated.py", "unauthorized custom edit\n") + tampered = self.commit("tampered candidate") + result, _ = budget.evaluate( + self.repo, + legacy, + tampered, + public=True, + target_branch="next", + policy_sha=main, + main_snapshot=main, + repository=repository, + ) + self.assertEqual(result["checks"]["isolation"]["state"], "failure") + self.assertEqual(result["checks"]["budget"]["state"], "failure") + + def test_activation_accepts_stale_pr_without_policy_edits(self) -> None: + strict = (self.repo / budget.POLICY).read_text() + self.git("checkout", "-q", "-b", "migration", f"{self.base}^") + self.write(budget.POLICY, '{"schema_version":1,"max_custom_patch_lines":1000}') + migration = self.commit("migration policy before activation") + self.git("checkout", "-q", "-b", "stale", migration) + self.write("handwritten.py", "custom behavior\n") + stale = self.commit("PR predates strict activation") + self.git("checkout", "-q", "migration") + self.write(budget.POLICY, strict) + activated = self.commit("activate strict policy") + result, _ = budget.evaluate(self.repo, activated, stale, public=False) + self.assertTrue(all(c["state"] == "success" for c in result["checks"].values())) + self.git("merge", "--no-ff", "-m", "queue stale PR", stale) + result, _ = budget.evaluate( + self.repo, + activated, + self.git("rev-parse", "HEAD"), + public=False, + pull_heads=[stale], + ) + self.assertTrue(all(c["state"] == "success" for c in result["checks"].values())) + + def test_stale_policy_edit_cannot_downgrade_activated_base(self) -> None: + strict = (self.repo / budget.POLICY).read_text() + self.git("checkout", "-q", "-b", "migration", f"{self.base}^") + self.write(budget.POLICY, '{"schema_version":1,"max_custom_patch_lines":10}') + migration = self.commit("migration policy before activation") + self.git("checkout", "-q", "-b", "stale", migration) + self.write(budget.POLICY, '{"schema_version":1,"max_custom_patch_lines":1000}') + stale = self.commit("stale policy increase") + self.git("checkout", "-q", "migration") + self.write(budget.POLICY, strict) + activated = self.commit("activate strict policy") + result, _ = budget.evaluate(self.repo, activated, stale, public=False) + self.assertEqual(result["checks"]["isolation"]["state"], "failure") + self.assertIn("cannot be downgraded", result["checks"]["isolation"]["description"]) + + def test_workflow_edit_fails_despite_zero_legacy_patch(self) -> None: + self.write(self.workflow, "edited workflow\n") + result = self.evaluate() + self.assertEqual(result["total"], 0) + self.assertEqual(result["checks"]["budget"]["state"], "failure") + self.assertIn(self.workflow, result["strict_violations"][0]) + + def test_merge_queue_checks_combined_generated_ownership(self) -> None: + self.git("checkout", "-q", "-b", "handwritten", self.base) + self.write("handwritten.py", "custom behavior\n") + handwritten = self.commit() + self.git("checkout", "-q", "-b", "workflow", self.base) + self.write(self.workflow, "manual workflow edit\n") + workflow = self.commit() + self.git("checkout", "-q", "-b", "queue", self.base) + self.git("merge", "--no-ff", "-m", "queue handwritten", handwritten) + passing, _ = budget.evaluate( + self.repo, + self.base, + self.git("rev-parse", "HEAD"), + public=False, + pull_heads=[handwritten], + ) + self.assertTrue(all(c["state"] == "success" for c in passing["checks"].values())) + self.git("merge", "--no-ff", "-m", "queue workflow", workflow) + failing, _ = budget.evaluate( + self.repo, + self.base, + self.git("rev-parse", "HEAD"), + public=False, + pull_heads=[handwritten, workflow], + ) + self.assertEqual(failing["total"], 0) + self.assertEqual(failing["checks"]["isolation"]["state"], "success") + self.assertEqual(failing["checks"]["budget"]["state"], "failure") + self.assertIn(self.workflow, failing["strict_violations"][0]) + + def test_content_deletion_rename_mode_and_symlink_fail(self) -> None: + for mutation in ("edit", "delete", "rename", "mode", "symlink"): + with self.subTest(mutation=mutation): + self.git("reset", "--hard", self.base) + path = self.repo / "generated.py" + if mutation == "edit": + path.write_text("header removed and manually changed\n") + elif mutation == "delete": + path.unlink() + elif mutation == "rename": + path.rename(self.repo / "renamed.py") + elif mutation == "mode": + path.chmod(0o755) + else: + path.unlink() + path.symlink_to("handwritten.py") + self.assertEqual(self.evaluate()["checks"]["budget"]["state"], "failure") + + def test_candidate_cannot_rewrite_its_manifest_or_disable_strict(self) -> None: + self.write(self.workflow, "custom\n") + self.manifest() + self.write(budget.POLICY, json.dumps({"schema_version": 1, "max_custom_patch_lines": 1000})) + result = self.evaluate() + self.assertEqual(result["checks"]["isolation"]["state"], "failure") + self.assertEqual(result["checks"]["budget"]["state"], "failure") + + def test_missing_manifest_cannot_activate(self) -> None: + self.git("checkout", "-q", self.generated) + (self.repo / budget.MANIFEST.decode()).unlink() + checkpoint = self.commit(self.git("show", "-s", "--format=%B", self.generated)) + self.git("update-ref", "refs/remotes/origin/codegen/test", checkpoint) + self.git("checkout", "-q", "main") + stats = (self.repo / ".castiron.stats.yml").read_text() + self.write( + ".castiron.stats.yml", + stats.replace(self.generated, checkpoint).replace( + report.hash_codegen_commit(self.repo, self.generated), + report.hash_codegen_commit(self.repo, checkpoint), + ), + ) + self.assertEqual(self.evaluate()["checks"]["budget"]["state"], "failure") + + def test_legitimate_new_generated_checkpoint_passes(self) -> None: + self.git("checkout", "-q", self.generated) + self.write("generated.py", "new generated output\n") + self.write(self.workflow, "new generated workflow\n") + self.manifest() + checkpoint = self.commit(self.git("show", "-s", "--format=%B", self.generated)) + self.git("update-ref", "refs/remotes/origin/codegen/test", checkpoint) + self.git("checkout", "-q", "main") + for path in ("generated.py", self.workflow, budget.MANIFEST.decode()): + self.write(path, self.git("show", f"{checkpoint}:{path}") + "\n") + # Preserve exact manifest bytes (the fixture writes JSON without a newline). + self.write( + budget.MANIFEST.decode(), self.git("show", f"{checkpoint}:{budget.MANIFEST.decode()}") + ) + stats = (self.repo / ".castiron.stats.yml").read_text() + self.write( + ".castiron.stats.yml", + stats.replace(self.generated, checkpoint).replace( + report.hash_codegen_commit(self.repo, self.generated), + report.hash_codegen_commit(self.repo, checkpoint), + ), + ) + self.assertEqual(self.evaluate()["checks"]["budget"]["state"], "success") + + def test_worktree_verification_catches_uncommitted_edits(self) -> None: + self.assertEqual( + budget.verify_generated(self.repo, self.generated, self.base, worktree=True), [] + ) + self.write(self.workflow, "uncommitted\n") + failures = budget.verify_generated(self.repo, self.generated, self.base, worktree=True) + self.assertEqual(len(failures), 1) + self.assertIn(self.workflow, failures[0]) + + def test_activation_is_policy_only_and_checks_workflows(self) -> None: + strict = (self.repo / budget.POLICY).read_text() + self.write(budget.POLICY, '{"schema_version":1,"max_custom_patch_lines":1000}') + migration = self.commit() + self.write(budget.POLICY, strict) + activation = self.commit() + result, _ = budget.evaluate(self.repo, migration, activation, public=False) + self.assertTrue(all(check["state"] == "success" for check in result["checks"].values())) + self.write(self.workflow, "custom workflow\n") + result, _ = budget.evaluate(self.repo, migration, self.commit(), public=False) + self.assertTrue(all(check["state"] == "failure" for check in result["checks"].values())) + + def test_manifest_must_cover_snapshot_and_match_its_bytes(self) -> None: + self.git("checkout", "-q", self.generated) + manifest = json.loads((self.repo / budget.MANIFEST.decode()).read_text()) + manifest["files"].pop("generated.py") + self.write(budget.MANIFEST.decode(), json.dumps(manifest)) + with self.assertRaisesRegex(ValueError, "cover.*exactly"): + budget.generated_manifest(self.repo, self.commit()) + self.manifest() + self.write("generated.py", "snapshot changed without updating manifest\n") + with self.assertRaisesRegex(ValueError, "disagrees"): + budget.generated_manifest(self.repo, self.commit()) + + def test_relocated_tooling_preserves_ownership_and_retirement_checks(self) -> None: + self.git("checkout", "--detach", "-q", self.generated) + old_manifest = budget.MANIFEST.decode() + new_manifest = "generated/castiron/generated-files.json" + old_checker = "scripts/castiron/custom_code_budget.py" + new_checker = "generated/castiron/custom_code_budget.py" + manifest = json.loads((self.repo / old_manifest).read_text()) + manifest["files"][new_checker] = manifest["files"].pop(old_checker) + checker = (self.repo / old_checker).read_text() + self.write(new_checker, checker) + self.write(new_manifest, json.dumps(manifest)) + (self.repo / old_checker).unlink() + (self.repo / old_manifest).unlink() + relocated = self.commit("relocate generated tooling") + self.assertEqual( + budget.verify_generated(self.repo, relocated, relocated, previous=self.generated), [] + ) + ownership = budget.generated_manifest(self.repo, relocated) + self.assertIn(new_manifest.encode(), ownership) + self.assertIn(new_checker.encode(), ownership) + self.assertNotIn(old_manifest.encode(), ownership) + self.write(old_checker, checker) + self.write("generated.py", "handwritten modification\n") + failures = budget.verify_generated( + self.repo, relocated, self.commit(), previous=self.generated + ) + self.assertTrue( + any(old_checker in failure and "retired" in failure for failure in failures) + ) + self.assertTrue(any("generated.py: differs" in failure for failure in failures)) + # Candidate manifest edits cannot change the verified ownership contract. + self.write(new_manifest, '{"schema_version": 1, "files": {}}') + failures = budget.verify_generated(self.repo, relocated, self.commit()) + self.assertTrue( + any(new_manifest in failure and "differs" in failure for failure in failures) + ) + + def test_checkpoint_rejects_ambiguous_manifest_locations(self) -> None: + self.git("checkout", "--detach", "-q", self.generated) + self.write( + "generated/castiron/generated-files.json", + (self.repo / budget.MANIFEST.decode()).read_text(), + ) + with self.assertRaisesRegex(ValueError, "ambiguous"): + budget.generated_manifest(self.repo, self.commit()) + + def test_relocated_manifest_requires_colocated_checker_inventory(self) -> None: + self.git("checkout", "--detach", "-q", self.generated) + self.write( + "generated/castiron/generated-files.json", + (self.repo / budget.MANIFEST.decode()).read_text(), + ) + (self.repo / budget.MANIFEST.decode()).unlink() + with self.assertRaisesRegex(ValueError, "missing its generated GitHub inventory"): + budget.generated_manifest(self.repo, self.commit()) + + def test_manifest_preserves_inherited_sdk_owned_github_files(self) -> None: + self.git("checkout", "--detach", "-q", self.generated) + inherited = (".github/workflows/ci.yml", ".github/actions/setup-go/action.yml") + for path in inherited: + self.write(path, "SDK-owned configuration\n") + generated = self.commit("checkpoint preserves protected parent subtrees") + manifest = budget.generated_manifest(self.repo, generated) + for path in inherited: + self.assertNotIn(path.encode(), manifest) + self.write(path, "SDK-owned configuration update\n") + head = self.commit("handwritten configuration remains outside generated ownership") + self.assertEqual(budget.verify_generated(self.repo, generated, head), []) + + def test_private_manifest_checks_excluded_workflows_and_actions(self) -> None: + self.assertIn(self.workflow.encode(), budget.GENERATED_GITHUB_PATHS) + for path in (self.workflow, ".github/actions/generated/action.yml"): + for mutation in ("omit", "content", "mode"): + with ( + self.subTest(path=path, mutation=mutation), + mock.patch.object( + budget, + "GENERATED_GITHUB_PATHS", + budget.GENERATED_GITHUB_PATHS | {path.encode()}, + ), + ): + self.git("checkout", "--detach", "-q", self.generated) + self.write(path, "generated workflow or action\n") + self.inventory(budget.GENERATED_GITHUB_PATHS) + self.manifest((path,)) + manifest = json.loads((self.repo / budget.MANIFEST.decode()).read_text()) + manifest["files"][path] = { + "mode": "100644", + "sha256": hashlib.sha256((self.repo / path).read_bytes()).hexdigest(), + } + if mutation == "omit": + del manifest["files"][path] + elif mutation == "content": + self.write(path, "unrecorded change\n") + else: + (self.repo / path).chmod(0o755) + self.write(budget.MANIFEST.decode(), json.dumps(manifest)) + error = ( + "omits generated GitHub inventory path" + if mutation == "omit" + else "disagrees" + ) + with self.assertRaisesRegex(ValueError, error): + budget.generated_manifest(self.repo, self.commit()) + + def test_checkpoint_inventory_can_add_and_retire_generated_ci(self) -> None: + for path in (".github/workflows/new.yml", ".github/actions/new/action.yml"): + with self.subTest(path=path): + self.git("checkout", "--detach", "-q", self.generated) + self.write(path, "new generated CI\n") + self.inventory(budget.GENERATED_GITHUB_PATHS | {path.encode()}) + self.manifest((path,)) + newer = self.commit("new compiler inventory") + # The executing verifier still has the old inventory. + self.assertNotIn(path.encode(), budget.GENERATED_GITHUB_PATHS) + self.assertEqual( + budget.verify_generated(self.repo, newer, newer, previous=self.generated), [] + ) + (self.repo / path).unlink() + public = self.commit("public checkpoint omits private CI blob") + self.assertEqual( + budget.verify_generated(self.repo, public, newer, previous=self.generated), [] + ) + # A newer verifier must also read the prior inventory to enforce retirement. + self.assertEqual( + budget.verify_generated(self.repo, self.generated, self.base, previous=public), + [], + ) + self.git("checkout", "--detach", "-q", self.base) + self.write(path, "retired generated CI\n") + failures = budget.verify_generated( + self.repo, self.generated, self.commit(), previous=public + ) + self.assertTrue( + any(path in failure and "retired" in failure for failure in failures) + ) + + def test_manifest_cannot_claim_sdk_owned_workflows_or_actions(self) -> None: + for path in (".github/workflows/ci.yml", ".github/actions/setup-go/action.yml"): + for keep_blob in (True, False): + with self.subTest(path=path, keep_blob=keep_blob): + self.git("checkout", "--detach", "-q", self.generated) + self.write(path, "SDK-owned configuration\n") + manifest = json.loads((self.repo / budget.MANIFEST.decode()).read_text()) + manifest["files"][path] = { + "mode": "100644", + "sha256": hashlib.sha256((self.repo / path).read_bytes()).hexdigest(), + } + if not keep_blob: + (self.repo / path).unlink() + self.write(budget.MANIFEST.decode(), json.dumps(manifest)) + with self.assertRaisesRegex(ValueError, "SDK-owned GitHub path"): + budget.generated_manifest(self.repo, self.commit()) + + def test_public_snapshot_requires_every_inventory_identity(self) -> None: + for path in (self.workflow, ".github/actions/generated/action.yml"): + with self.subTest(path=path): + self.git("checkout", "--detach", "-q", self.generated) + self.write(path, "generated CI\n") + self.inventory(budget.GENERATED_GITHUB_PATHS | {path.encode()}) + self.manifest((path,)) + (self.repo / path).unlink() + manifest = json.loads((self.repo / budget.MANIFEST.decode()).read_text()) + del manifest["files"][path] + self.write(budget.MANIFEST.decode(), json.dumps(manifest)) + with self.assertRaisesRegex(ValueError, "omits generated GitHub inventory path"): + budget.generated_manifest( + self.repo, self.commit("incomplete public checkpoint") + ) + + def test_public_snapshot_keeps_strict_workflow_identity_without_private_blob(self) -> None: + self.git("checkout", "--detach", "-q", self.generated) + (self.repo / self.workflow).unlink() + public = self.commit("sanitized public checkpoint") + self.assertIn(self.workflow.encode(), budget.generated_manifest(self.repo, public)) + self.assertEqual(budget.verify_generated(self.repo, public, self.base), []) + self.git("checkout", "--detach", "-q", self.base) + self.write(self.workflow, "manual workflow edit\n") + violations = budget.verify_generated(self.repo, public, self.commit()) + self.assertEqual(len(violations), 1) + self.assertIn(self.workflow, violations[0]) + + def test_generated_deletion_requires_removing_retired_file(self) -> None: + self.git("checkout", "-q", self.generated) + (self.repo / "generated.py").unlink() + manifest = json.loads((self.repo / budget.MANIFEST.decode()).read_text()) + del manifest["files"]["generated.py"] + self.write(budget.MANIFEST.decode(), json.dumps(manifest)) + new = self.commit() + self.assertEqual(budget.verify_generated(self.repo, new, new, previous=self.generated), []) + self.write("generated.py", "retained handwritten replacement\n") + failures = budget.verify_generated(self.repo, new, self.commit(), previous=self.generated) + self.assertEqual(len(failures), 1) + self.assertIn("retired generated file", failures[0]) + + def test_generated_file_can_become_a_directory(self) -> None: + self.git("checkout", "-q", self.generated) + (self.repo / "generated.py").unlink() + self.write("generated.py/nested.py", "new generated file\n") + manifest = json.loads((self.repo / budget.MANIFEST.decode()).read_text()) + del manifest["files"]["generated.py"] + manifest["files"]["generated.py/nested.py"] = { + "mode": "100644", + "sha256": hashlib.sha256(b"new generated file\n").hexdigest(), + } + self.write(budget.MANIFEST.decode(), json.dumps(manifest)) + new = self.commit() + for worktree in (False, True): + with self.subTest(worktree=worktree): + self.assertEqual( + budget.verify_generated( + self.repo, new, new, previous=self.generated, worktree=worktree + ), + [], + ) + + def test_compiler_entrypoint_preserves_failed_worktree(self) -> None: + self.write(self.workflow, "uncommitted custom workflow\n") + result = subprocess.run( + [ + sys.executable, + "-I", + str(Path(budget.__file__)), + "verify-generated", + "--repo", + str(self.repo), + "--policy-ref", + self.base, + "--generated", + self.generated, + "--head", + self.base, + "--worktree", + ], + capture_output=True, + text=True, + check=False, + ) + self.assertEqual(result.returncode, 1) + self.assertIn(self.workflow, result.stderr) + self.assertEqual((self.repo / self.workflow).read_text(), "uncommitted custom workflow\n") + + +if __name__ == "__main__": + unittest.main() diff --git a/src/openai/_client.py b/src/openai/_client.py index 2a10cd855a..7dadc8b4ca 100644 --- a/src/openai/_client.py +++ b/src/openai/_client.py @@ -260,9 +260,16 @@ def _refresh_api_key( def _websocket_query_params_fallback(self, url: httpx2.URL, extra_query: Query) -> httpx2.QueryParams: # Remove replaced logical defaults before encoding nested query values. overridden = dict.fromkeys(extra_query, omit) + overridden_prefixes = tuple(f"{key}[" for key in overridden) defaults = _merge_mappings(self.default_query, overridden) params = httpx2.QueryParams(self.qs.stringify(cast(Any, defaults))).merge(url.params) - params = httpx2.QueryParams([(key, value) for key, value in params.multi_items() if key not in overridden]) + params = httpx2.QueryParams( + [ + (key, value) + for key, value in params.multi_items() + if key not in overridden and not key.startswith(overridden_prefixes) + ] + ) if isinstance(extra_query, httpx2.QueryParams): return params.merge(extra_query) # Per-call None and sequences retain blank values and repeated keys; @@ -780,9 +787,16 @@ async def _refresh_api_key( def _websocket_query_params_fallback(self, url: httpx2.URL, extra_query: Query) -> httpx2.QueryParams: # Remove replaced logical defaults before encoding nested query values. overridden = dict.fromkeys(extra_query, omit) + overridden_prefixes = tuple(f"{key}[" for key in overridden) defaults = _merge_mappings(self.default_query, overridden) params = httpx2.QueryParams(self.qs.stringify(cast(Any, defaults))).merge(url.params) - params = httpx2.QueryParams([(key, value) for key, value in params.multi_items() if key not in overridden]) + params = httpx2.QueryParams( + [ + (key, value) + for key, value in params.multi_items() + if key not in overridden and not key.startswith(overridden_prefixes) + ] + ) if isinstance(extra_query, httpx2.QueryParams): return params.merge(extra_query) # Per-call None and sequences retain blank values and repeated keys;