Repository navigation
chore: strengthen generated file verification - #1192
Conversation
Castiron custom codeEvaluated main: ✅ No new custom-code files detected. 102 mixed files remain; 0 existing customizations changed; 3 customizations removed; 3 generated baselines changed. Compared
102 existing customizations unchanged
62 more in the full report. A changed generated baseline means this report cannot reliably identify which handwritten lines changed. Inspect the custom-code diffDownload the exact patch produced by this run (requires repository access): gh run download 38845713798 --repo openai/openai-java \
--name castiron-custom-code-38845713798-1 --dir /tmp/castiron-custom-code-38845713798-1
git apply --stat /tmp/castiron-custom-code-38845713798-1/custom-code.patch
cat /tmp/castiron-custom-code-38845713798-1/custom-code.patchOr reproduce it from an SDK checkout containing the vendored reporter: git fetch --no-tags origin af7ce3bde09eb5b256d9b4fc4e6a4406e2da4dde 55d3fa834b8d1db9530a0bac18827dd991551f3f
python3 scripts/castiron/custom_code_report.py report \
--base af7ce3bde09eb5b256d9b4fc4e6a4406e2da4dde \
--head 55d3fa834b8d1db9530a0bac18827dd991551f3f --fetch --require-head-hash --public \
--out /tmp/castiron-custom-code-55d3fa834b8d
cat /tmp/castiron-custom-code-55d3fa834b8d/custom-code.patchThis is the current full custom patch for mixed files, not an attribution of only the handwritten lines changed by this PR. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Reviewed all 12 changed files at aef9cd32f251608f17f88a4c3511980b9a4c51be. Requesting changes for two target/freshness correctness issues in the newly added next status support: main and next results share SHA-scoped contexts, and a next queue result can outlive the main policy it evaluated. Keeping next advisory would avoid both problems until authoritative next checks are coupled correctly.
The generated-file verifier, provenance handling, and existing schema-v1 migration policy otherwise look consistent. I checked all 3,776 manifest entries; the 110 byte differences correspond to existing mixed-file customizations, not newly introduced drift. Runtime/API sources, dependencies, and the budget policy are unchanged.
Static review only; no candidate code or tests run locally. Current exact-head hosted CI has 15 successful checks/statuses and 2 skipped checks, with no failures or pending checks.
| const {data: pr} = await github.rest.pulls.get({...context.repo, pull_number: pull.number}); | ||
| if (pr.state === 'open' && pr.head.sha === head && | ||
| pr.base.ref === 'main' && pr.base.repo.full_name === `${context.repo.owner}/${context.repo.repo}`) current.push(pr); | ||
| ['main', 'next'].includes(pr.base.ref) && pr.base.repo.full_name === `${context.repo.owner}/${context.repo.repo}`) current.push(pr); |
There was a problem hiding this comment.
[P2] Keep target-specific results out of the same status context
This now accepts both main and next, but the publisher below still keys both results by the same head SHA and the same two context names. The results can legitimately differ: if next already contains a policy-only commit B and H adds code on top of B, isolation passes against next but fails against main, which sees the mixed policy/code diff. Retargeting H to next lets its later success replace the main failure; retargeting it back leaves the wrong target's result displayed until a new evaluation finishes. The ordering marker also ignores target identity. Please keep next advisory for now or use target-specific status contexts/ordering, with a same-head retargeting regression case. This is status correctness feedback, not a claim that the separate main merge-queue check can be bypassed.
| const {data: target} = await github.rest.git.getRef({...context.repo, ref: `heads/${branch}`}); | ||
| const {data: main} = branch === 'main' ? {data: target} | ||
| : await github.rest.git.getRef({...context.repo, ref: 'heads/main'}); | ||
| fresh = fresh && base === target.object.sha && policy === main.object.sha; | ||
| fresh = fresh && member; |
There was a problem hiding this comment.
[P2] Invalidate next queue results when the main policy changes
This checks main only immediately before writing the status. Once a next queue head has passed under policy P1, it may still be waiting for another required check when main changes to P2. That does not change the next base or queue SHA, and neither workflow is triggered to invalidate/recompute this result, so the queue can retain a passing budget result for the old policy. This matters when the new CUSTOM_CODE.md activation steps make these statuses required on next. Please keep next non-authoritative until main-policy changes invalidate/re-evaluate its queue results, or implement that coupling before recommending activation. Add a case where main changes after status publication while next is still waiting.
markstuart-oai
left a comment
markstuart-oai
left a comment
There was a problem hiding this comment.
Reviewed all 12 changed files at aef9cd32f251608f17f88a4c3511980b9a4c51be. I agree with the existing policy-invalidation finding and am leaving this review without approval.
The pre-publication comparison only proves that main policy is current when a next-queue status is written. If main later tightens that policy while the same next-queue head waits for another check, neither workflow invalidates or recomputes the saved success. The new activation guidance would make that stale result authoritative. Keep next explicitly advisory, with separate status labels and narrowed activation guidance, until policy changes can invalidate or re-evaluate pending next-queue results. A regression should cover success, a subsequent main-policy change, and delayed queue completion. I have no evidence that authoritative next checks are currently enabled.
The shared-context finding also describes a real retargeting display problem; the fresh main-queue check prevents a demonstrated main merge bypass. I am referencing both existing threads rather than posting duplicate inline comments.
The generated-file verifier, public checkpoint identity, unchanged schema-v1 budget and ownership policy otherwise look consistent. Exact-head CI has 15 successful and two skipped checks. Source and hosted-CI review only; no candidate code or local tests ran.
Castiron-Internal-PR: openai/openai-java-internal#276 Castiron-Source-SHA: a22be628beb6353aec408e6490c8e1dbeef8da2a Castiron-Public-Base-SHA: af7ce3b
aef9cd3 to
55d3fa8
Compare
markstuart-oai
left a comment
markstuart-oai
left a comment
There was a problem hiding this comment.
Re-reviewed all 12 changed files at 55d3fa834b8d1db9530a0bac18827dd991551f3f. Both earlier findings are fixed. The publisher returns for next before reading or writing the main status contexts, and the guidance now keeps next advisory until separate authorization and policy invalidation exist. The added tests cover same-SHA main/next results and later main-policy changes.
The new inventory reader parses literal data from the verified checkpoint without executing it. It preserves generated-file ownership across compiler versions, rejects claims on SDK-owned CI, and requires identities for generated workflows omitted from public snapshots. The new ownership, retirement, and branch-rule pagination cases look consistent. I found no additional actionable issues.
The 3,776 manifest paths are unchanged; the seven updated identities match the tooling and documentation changes. The public checkpoint identity and unchanged SDK core subtree are verified. The schema-v1 budget and CODEOWNERS remain unchanged; this approval does not activate strict mode or next-branch enforcement.
At the final CI read, 13 checks/statuses passed, two were skipped, and CI / tests was still running. The changed-tooling baseline checks, budget statuses, API compatibility, and Java 8/25 runtime checks passed. Source and hosted-CI review only; no candidate code or local tests ran.
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Re-reviewed all 12 changed files at 55d3fa834b8d1db9530a0bac18827dd991551f3f. Both of my earlier findings are fixed: next returns before reading or writing the main status contexts, and the activation guidance keeps next advisory until separate contexts and policy invalidation exist. The same-SHA and stale-policy regression cases cover the corrected boundary. This approval supersedes my previous request for changes.
No additional actionable findings. I also reviewed the checkpoint-owned inventory parser, generated-file ownership/retirement checks, report identity binding, and pagination changes. All 3,776 manifest entries were checked as data; the seven updated identities match the changes, and the 102 candidate/generated byte differences are preexisting customizations. The public checkpoint hash and reporter checksum match. Runtime/API sources, dependencies, CODEOWNERS, and the schema-v1 budget policy are unchanged.
Exact-head hosted CI is complete: 15 successful checks/statuses and 2 intentionally skipped queue-signal checks; none failed or pending. Static source review and inert hashing only—no candidate code or tests executed locally.
3 participants
Update the shared SDK maintenance tooling to verify generated file contents, modes, workflow ownership, and retired paths when a repository opts into strict ownership. The repository keeps its existing schema-v1 policy and unchanged budget; this PR does not activate strict mode.
Evaluate
mainandnextagainst trustedmainpolicy; publish required statuses only formain, leavingnextadvisory.The API snapshot, runtime source, public types, dependency manifests, and repository policies are unchanged.
Validation: all 95 local checker and publisher tests passed, including the compiler hash contract and same-SHA and
nextqueue regressions; the workflow reporter checksum matches the integrated file. Scope and whitespace checks passed. Hosted CI and SDK CODEOWNER review remain required.