Repository navigation
[WRONG BRANCH] fix(responses): refuse an account change that would orphan an uploaded file (#4710) - #4777
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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. |
…4710) [skip ci] The classifier has always called an uploaded file_id account-bound, and the scrubber has always removed only previous_response_id and conversation. A body whose only account-bound state was a file reference therefore reported nothing scrubbed and was replayed unchanged against the new account, which is exactly the case the advertised safety fix was supposed to cover. Deleting the references is not the fix. A file reference is content the caller attached, not continuation state the turn can do without, and dropping it silently answers a different question than the one that was asked with no way for the caller to tell. Pinning the request to the issuing account is not available either: every call site resolves and materialises its credential before reaching here, and the retry sites are reached precisely because the issuing account just refused the request. So the move is refused before dispatch, with HTTP 400 and an instruction to re-upload. Not a retryable status, which would invite the same request back unchanged. The check reads the carriers directly rather than the portability verdict. That verdict reports the FIRST reason it finds, so a body carrying both a previous response id and a file reference reports only the response id, and the file would slip through the scrub that follows. Wired at the initial Codex selection and at the native compact dispatch, both of which can answer with a Response. The two alternate-account retry sites still only scrub: refusing there needs a new outcome variant on their result types and on their callers, which is a larger change than this one. The detection now lives in one exported place, so neither site can drift further from it. Closes #4710
리뷰 · 우선순위 73 / 80이 PR은 이슈 이 PR은 같은 버그를 고치려다 머지 없이 닫힌 alternate 경로도 이번에 막습니다. 스택은 다만 compact 초기 디스패치 가드에 실수가 있습니다. 라인 compact.ts 초기 가드( 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
…t paths too (#4710) The carried change refuses an uploaded-file move at the two sites that can answer with a status, and leaves the two alternate-account retry sites scrubbing only. This closes those two, and it does not need the new result variant the deferral assumed. A retry site does not have to raise a status, because an earlier response already exists and is what the caller returns. It only has to decline the move. And the question it declines on does not depend on which alternate would be chosen -- an uploaded file is readable only by the account that received it -- so it can be asked from the body alone, before an alternate is resolved. conversationCarriesUploadedFiles is that predicate. Asking there reserves no send, cancels no response, and leaves the first account's rejection intact for the caller, which is the guarantee the comment above the compact resolution already depended on. Refusal still beats a previous_response_id at both sites, for the same reason the carried change reads the carriers directly instead of the portability verdict: the verdict reports only the first denial it finds. Two corrections to the carried code. collectConversationStateCarriers(body).fileIds is optional on the carrier type and was dereferenced directly, which does not survive a strict typecheck; the emptiness test now lives in one exported place so no caller can get it wrong again. And the refusal message named the cause but not the consequence. The reference stays in the conversation's history, so once rotation has moved a conversation carrying an attachment, every later turn is refused the same way. A caller told only that the reference is invalid resends unchanged and watches the conversation die. The message now says what happened, that it will keep happening, and the two things that end it: re-upload under the serving account, or start a new conversation. A same-account replay is unaffected, and a single-account install never reaches any of this, because serving and issuing accounts cannot differ without pool rotation. Pinning a file-carrying conversation to its issuing account is the real answer and is routing-affinity work; filed as #4778. Co-authored-by: JUN <bitkyc08@gmail.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9273e66014
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| if (conversationCarriesUploadedFiles(raw) || conversationCarriesUploadedFiles(compactBody)) { | ||
| logCtx.conversationStateScrub = "account-change"; | ||
| return formatErrorResponse(409, "account_change_file_scope", ACCOUNT_CHANGE_FILE_SCOPE_MESSAGE); | ||
| } |
There was a problem hiding this comment.
When a pooled /v1/responses/compact request contains any file_id, this condition returns 409 account_change_file_scope merely because a pool binding exists; it never verifies that the serving account differs from the recorded issuer. Consequently, compacting a conversation on the same account that owns its uploaded file—and even the first request before an issuer is recorded—is blocked. Let applyAccountChangeConversationStateScrub perform its issuer comparison for both bodies and return 409 only if either outcome is refused.
Useful? React with 👍 / 👎.
| if (outcome.kind === "refused") { | ||
| return formatErrorResponse(409, "account_change_file_scope", ACCOUNT_CHANGE_FILE_SCOPE_MESSAGE); |
There was a problem hiding this comment.
When the newly selected account is carrying a cooldown probe lease and the uploaded-file check refuses the request, this return occurs before any upstream dispatch but does not release that lease. With the default disabled host circuit, pendingHostAdmissionLease is null, so the core.ts cleanup only guarded by that value also skips releaseCodexAuthContextProbeLease; the account then retains an in-flight probe marker and cannot receive another recovery probe for the remaining cooldown. Release the selected auth context's probe lease on this local-refusal path.
Useful? React with 👍 / 👎.
9273e66 to
3ae8ca7
Compare
|
✅ Deterministic PR hygiene checks passed. |
|
Landing the upstream quota-classification and account-change file-scope lane (three layers). Maintainer integration decision under MAINTAINERS.md / AGENTS.md: a maintainer with maintain or admin access may integrate a pull request into Evidence at the exact head 3ae8ca7:
Merge commit (not squash) is used deliberately to preserve the cumulative layer ancestry, so #4764 and #4765 close as merged. Contents: #4764 reclassifies the upstream 429 organization and project spend-limit codes as terminal quota exhaustion so the pool stops rotating to accounts that share the same limit; #4765 corrects the identity-domain comment to quote the sentence the referenced page actually contains; #4777 refuses an account change that would carry an uploaded file reference to an account that cannot read it, without ever deleting the reference. |
44392b5
into
codex/cx2-identity-domain-doc-source
⏳ DRAFT
What to do
UI screenshot waived by a maintainer comment. |
github-actions
Bot
changed the title
…ange-file-scope fix(responses): refuse an account change that would orphan an uploaded file (lidge-jun#4710)
1 participant
Summary
src/server/responses/account-change-state.tsclassified an uploadedfile_idas account-bound and then forwarded it anyway. The scrub removes onlyprevious_response_idandconversation, so a body whose only account-bound state was a file reference reported nothing scrubbed and went to the new account unchanged -- the one case the earlier safety fix was meant to cover.This branch carries the fix from
codex/bl6-account-change-file-scope(commitc22b626bd9, authored there and preserved by cherry-pick; its PR was closed) and completes the two call sites that commit deferred.The contract
Deleting the reference is not available: a file reference is content the caller attached, not continuation state the turn can do without, and dropping it silently answers a different question than the one that was asked. Sending it is not available either, because the new account answers with an opaque inaccessible-file error. So the move is refused, before dispatch, and the body is left intact.
accountChangeFileReferenceRefusalreads the carriers directly rather than through the portability verdict, because that verdict reports only the first denial it finds: a body carrying both a previous response id and a file reference would report only the response id, and the file would slip through the scrub that follows.The initial
/v1/responsesselection and the native compact dispatch answer HTTP 400 -- not a retryable status, which would invite the same request straight back.What this adds on top of the carried commit
The two alternate-account retry sites are now wired. The carried commit deferred them on the grounds that refusing there needs a new outcome variant on the result types and their callers. It does not. A retry site never had to raise a status: an earlier response already exists and is what the caller returns, so the site only has to decline the move. And the question it declines on does not depend on which alternate would be chosen -- an uploaded file is readable only by the account that received it -- so it can be asked from the body alone, before an alternate is resolved.
conversationCarriesUploadedFilesis that predicate. Asking before resolution reserves no send, cancels no response, and leaves the first account's rejection intact for the caller, which is exactly the guarantee the existing comment above the compact alternate resolution already depends on. An earlier draft of mine placed the check after resolution and would have cancelled a response the caller still had to return; that draft was discarded.Two corrections to the carried code.
collectConversationStateCarriers(body).fileIdsis optional on the carrier type and was dereferenced directly, which does not survive a strict typecheck. That commit carried[skip ci], so nothing caught it. The emptiness test now lives in one exported place.Scope
Serving and issuing accounts can only differ under pool rotation, so a single-account install never reaches any of this. A same-account replay -- the gated-model 400 ladder -- is unaffected, because it never leaves the issuing account.
Pinning a file-carrying conversation to its issuing account is the real answer and is routing-affinity work outside this change. Filed as #4778.
Closes #4710.
Stacking
Targets
codex/cx2-identity-domain-doc-sourceand contains both earlier layers. Retarget todevonce the parents land or close. This is the lane tip, so its CI run is the lane's gate.Verification
Local verification was NOT run, by explicit instruction from the repository owner. No local suite, no individual test file, no typecheck, and no build were run. This push used
--no-verify. The only evidence is hosted CI at the exact head SHA of this branch. Note that the carried commit's own CI was skipped, which is how the optional-property dereference reached this branch at all.Static checks done instead: traced both retry sites to confirm the guard sits before the alternate is resolved and before any permit is reserved; confirmed the carried commit's existing tests make no assertion on the message text, so rewriting it breaks nothing; confirmed
structure/transports/responses.mdis already listed ingrace.oversizeDocsso its growth does not trip the 600-line rule inscripts/structure-ssot.ts.Coverage is in the existing
tests/responses/responses-account-change-scrub.test.ts, so no test-layout registration changes. The carried commit's five cases are unchanged. Added: the predicate answers from the body alone; a file behind aprevious_response_idis still detected, which is the case a verdict-based predicate would miss; bothfile_idandfile_idsshapes; an emptyfile_idsarray is not a reference; ordinary bodies still permit the move, since a predicate that answered true too often would silently disable alternate-account recovery; and the message names the cause, the persistence and both remedies.Checklist