Repository navigation
chore: refresh custom-code enforcement tooling - #1270
Conversation
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. |
Castiron custom codeEvaluated main: Trusted main policy: ✅ No new custom-code files detected. 0 mixed files remain; 0 existing customizations changed. Compared 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 39885435874 --repo openai/openai-java \
--name castiron-custom-code-39885435874-1 --dir /tmp/castiron-custom-code-39885435874-1
git apply --stat /tmp/castiron-custom-code-39885435874-1/custom-code.patch
cat /tmp/castiron-custom-code-39885435874-1/custom-code.patchOr reproduce it from an SDK checkout containing the vendored reporter: git fetch --no-tags origin 65bd9e403f8e7f576bf7de904ba7237b6142368f 1b993a86a66c41c04066f7a56fe0130ec9019bf4
python3 scripts/castiron/custom_code_report.py report \
--base 65bd9e403f8e7f576bf7de904ba7237b6142368f \
--head 1b993a86a66c41c04066f7a56fe0130ec9019bf4 --target-branch main --fetch --require-head-hash --public \
--out /tmp/castiron-custom-code-1b993a86a66c
cat /tmp/castiron-custom-code-1b993a86a66c/custom-code.patchThis is the current full custom patch for mixed files, not an attribution of only the handwritten lines changed by this PR. |
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Two P2 issues remain in the new review-change handler: a base advance can drop a legitimate dismissal before invalidation, and the filename-only policy gate invalidates changes that do not require human attestation. Please align authentication with the unchanged PR head and invalidation eligibility with the selector's semantic predicate, with regression coverage for both.
Reviewed the workflow trust boundaries, selector/publisher contracts, budget/adoption/report changes, and tests statically. Hosted CI: 20 checks successful, 3 skipped. No candidate code or tests executed.
markstuart-oai
left a comment
There was a problem hiding this comment.
Reviewed the Java workflow and policy changes at c8be35345aa3a8575d4a58cc7c6f061381f16e69. Two existing P2 findings remain:
- Preserve revocation after the target base advances: authenticate the historical review signal against the unchanged PR head before invalidating current successful statuses.
- Gate invalidation on changes that require attestation: budget decreases and strict-only edits should not enter the approval-revocation path.
Both need focused regressions. Static source review only; I did not run candidate code or tests. The existing security scanner does not support this immutable-blob layout, so no native scan result is claimed. Hosted checks succeeded or were skipped.
Castiron-Internal-PR: openai/openai-java-internal#349 Castiron-Source-SHA: 8f04b7124b1bf9fae2e7cfb5bf089c5a61be0365 Castiron-Public-Base-SHA: 65bd9e4
c8be353 to
1b993a8
Compare
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Approved at 1b993a8. Both prior P2 findings are addressed: associated historical merge signals authenticate the unchanged head and ancestor base, while branch-only fallback retains strict live-merge checks; invalidation now follows the selector's semantic attestation predicate. The new pin logic compares effective policy against the current protected target, preserves rollback/relaxation authorization, and rechecks changing refs before a no-op.
Re-reviewed all eight changed blobs against the prior reviewed commit, including surrounding workflow/selector code and regression tests. Other repository blobs are unchanged, and the six changed owned-file hashes match the generated manifest. No new actionable findings from independent static reviews.
Hosted CI has no failures; six jobs were still running at the latest check. Static review only; no candidate code or tests executed.
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Correcting my earlier approval: I missed a bootstrap case in the effective-policy comparison. The previous two findings are fixed, but the selector and review handler still compare against inactive, unpinned next policy fields during initial pin adoption. Please compare against the authenticated main policy actually executing during bootstrap and add regressions in both paths.
Hosted CI is complete: 18 successful checks, 5 skipped. Independent static inspection only; no candidate code or tests executed.
| ) | ||
| # Authored changes come from the merge base, but their safety must be | ||
| # measured against the protected target that will actually be replaced. | ||
| previous = read_policy(repo, policy) if "next_policy" in base_policy else base_policy |
There was a problem hiding this comment.
[P2] Compare bootstrap adoption against the active main policy
When next has no pin, policy is authenticated_main, but this fallback compares proposed policy against base_policy instead. For example, unpinned next can retain a 3999-line ceiling while authenticated main enforces 4000; a policy-only PR adding a pin to that same current main, without changing its local ceiling, is then treated as an effective budget increase and blocked for human attestation even though the effective policy is unchanged. The review handler repeats this at .github/workflows/castiron-custom-code-comment.yml:82 and can unnecessarily invalidate statuses. Read the policy at the active checker in bootstrap too (read_policy(repo, policy), and readPolicy(previousChecker) in the handler), preserving the rollback and authored-increase guards. Add selector and handler regressions with divergent stale unpinned next fields and a current-main pin.
Summary
Validation
PYTHONDONTWRITEBYTECODE=1 python3 -m unittest discover -s scripts/castiron -p 'test_custom_code*.py': 230 tests run, 1 skipped.git diff --checkpassed.Review focus
The trusted review-change workflow uses
actions: writeandstatuses: writefor review-triggered status reevaluation. Please review workflow authentication, authorization, write permissions, and the human-attestation boundary before merging.Prerequisite for #1268.