Repository navigation
feat(api): add delegation controls and prompt cache lifetimes - #4169
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. |
|
@codex review |
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 40024962482 --repo openai/openai-python \
--name castiron-custom-code-40024962482-1 --dir /tmp/castiron-custom-code-40024962482-1
git apply --stat /tmp/castiron-custom-code-40024962482-1/custom-code.patch
cat /tmp/castiron-custom-code-40024962482-1/custom-code.patchOr reproduce it from an SDK checkout containing the vendored reporter: git fetch --no-tags origin d622f28ad5cb3f947541936c0c139891211a42c5 39556068d75909f8275440b0a4a8f60e61e271f1
python3 scripts/castiron/custom_code_report.py report \
--base d622f28ad5cb3f947541936c0c139891211a42c5 \
--head 39556068d75909f8275440b0a4a8f60e61e271f1 --target-branch main --fetch --require-head-hash --public \
--out /tmp/castiron-custom-code-39556068d759
cat /tmp/castiron-custom-code-39556068d759/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: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Reviewed 72c7937f2dd8010d82d170e9b1bf7bb586e7cd5b. Requesting changes for the review-revocation race.
[P2] Retry stale main snapshots before abandoning review revocation. At .github/workflows/castiron-custom-code-comment.yml:127, requiresHuman can throw when review-required fetches a newer main than the captured SHA (custom_code_policy.py:572-573). Neither this await nor the helper catches that freshness error. The job exits before invalidating prior successful PR/queue policy statuses, so an approval dismissal/edit can leave the old authorization intact. Retry discovery on this explicit stale-main result, or safely invalidate authenticated affected statuses before exiting. Add coverage with existing successful statuses and main advancing between the read and fetch.
Hosted CI: failed: Castiron / reporter tests; still running: optional provider types (browser). Static code review only; no candidate code or local tests executed.
marcuswood-oai
left a comment
marcuswood-oai
left a comment
There was a problem hiding this comment.
I reproduced the failure path in the existing review-revocation finding. No additional findings; holding approval for that fix.
Focused SDK probes and local policy tests passed. The hosted reporter job remains failed on temporary-directory cleanup.
markstuart-oai
left a comment
markstuart-oai
left a comment
There was a problem hiding this comment.
Reviewed 72c7937f2dd8010d82d170e9b1bf7bb586e7cd5b. I confirmed the existing review-revocation finding: if main advances between getRef and the helper fetch, the uncaught stale-main error exits before prior successful PR and queue policy statuses are invalidated. Retry that specific stale snapshot or safely invalidate the affected statuses before exiting. I have not duplicated the existing comment. The new one-time creation type also accepts fractional run_at values despite the integer API contract; see inline feedback.
Source-only review; I did not run candidate code or tests. Reporter CI failed during temporary Git-directory cleanup; the check remains failed.
Castiron-Internal-PR: openai/openai-python-internal#334 Castiron-Source-SHA: 023996f5a4f33d99c1a0cdbc202d3ef33c191898 Castiron-Public-Base-SHA: d622f28
72c7937 to
3955606
Compare
|
@codex review Updated the candidate to retry review-policy evaluation when main advances during evaluation. Added the regression covering invalidation of existing successful statuses. One-time automation timestamps and generated test samples now use integers. Please review the current head again. |
|
|
||
| timeout: Override the client-level default timeout for this request, in seconds | ||
| """ | ||
| ... |
|
|
||
| timeout: Override the client-level default timeout for this request, in seconds | ||
| """ | ||
| ... |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Re-reviewed 3955606. The main-ref race is fixed: a policy failure caused by main advancing retries discovery against the fresh ref, while same-ref failures still fail closed. The regression covers invalidating PR and queue statuses and rerunning checks. The once-automation run_at annotations and examples now consistently use integers; the additional JSON union ordering change looks sound. No remaining blocking findings in this follow-up. Static review only; no local tests run. Hosted CI was still running at review time and will be followed up separately.
markstuart-oai
left a comment
markstuart-oai
left a comment
There was a problem hiding this comment.
Reviewed 39556068d75909f8275440b0a4a8f60e61e271f1. No blocking findings remain. The review-policy workflow now retries discovery when main changes during policy evaluation and still rejects errors when main is unchanged. The regression covers revoking PR and queue statuses before reruns. One-time automation creation now uses integer timestamps in both client modes, request types, and test samples. The response JSON union still supports both documented object and list shapes.
Source-only review; I did not run candidate code or tests. Current hosted checks: 27 successful, 23 skipped, and one still running.
Withdrawing my approval: I missed a second retry path in the review-policy workflow. After an iteration marks current-head statuses pending, a change to the effective base policy can make requiresReview false on retry. The early return then skips reruns for dirtyTargets and leaves the current head pending. A corrective comment review follows.
markstuart-oai
left a comment
markstuart-oai
left a comment
There was a problem hiding this comment.
Correcting my earlier review of 39556068d75909f8275440b0a4a8f60e61e271f1: I withdrew my approval after finding a second policy retry bug. The inline P2 explains how a next-base policy update can leave an unchanged PR head stuck pending: the retry decides no review is needed and returns without restarting checks it already invalidated. Preserve authenticated reruns for still-current dirty targets and add that transition to the regression test.
The integer timestamp and stale-main exception fixes remain valid. Source-only review; I did not run candidate code or tests. Current hosted checks are green: 31 successful and 29 skipped; they do not cover this transition.
| if (!requiresReview) { | ||
| const [{data: current}, {data: target}, main] = await Promise.all([ | ||
| github.rest.pulls.get({...context.repo, pull_number: pr.number}), | ||
| 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; | ||
| return; |
There was a problem hiding this comment.
[P2] Re-run still-current dirty targets before this return. An iteration can mark the PR head pending, then retry because next advanced. If that base update adopts the proposed policy pin, requiresReview becomes false while the PR head stays unchanged. This branch returns without dispatching the prior reruns, leaving that head pending indefinitely. Keep the existing authenticated target/rerun path for still-current dirtyTargets, even when no new human review is needed. Add a regression with unchanged PR head, changed base pin, and a true-to-false policy decision; do not rerun obsolete queue heads.
Summary
Adds agent scheduling APIs, Live delegation controls, and prompt cache lifetimes.
Changes