Repository navigation
[WRONG BRANCH] docs(routing): cite what the prompt-caching guide actually says (#4546) - #4765
Conversation
The cache rule's conclusion is right and its source was not. The comment claimed OpenAI "documents that changing keys inside one organization does not guarantee a hit", and the prompt-caching guide contains no such sentence: the phrase "API key" appears on that page zero times, so it never addresses two keys in one organization in either direction. What the page does say is the separating half, verbatim: "Caches are not shared across organizations and cannot be reused across regional processing boundaries." The nearest statement about keys is "Keys influence routing; they do not pin requests to a machine or guarantee a cache hit." The classification is unchanged. A different org or region still relates distinct, an identical one still relates unknown, and evidence stays "separates". Only the reason moves: same org and region is unknown because the provider never promised the hit, not because the provider denied it. A reader who went looking for the denial this comment described would not have found it, and would have had to guess whether the code or the comment was wrong. The OpenAI quota rule gains its verbatim source in the same pass -- "Rate limits are defined at the organization level and at the project level, not user level" -- which is what "separates-and-shares" rests on. structure/catalog.md carried the same mis-citation and is corrected to match. Sources read from the signed-in platform documentation on 2026-09-16.
|
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. |
리뷰 · 우선순위 77 / 80이 PR은 문제의 핵심은 이렇습니다. 예전 주석은 OpenAI 프롬프트 캐싱 가이드가 “같은 조직 안에서 키를 바꿔도 히트를 보장하지 않는다”고 문서에 적었다고 말했습니다. 그런데 그 페이지에는 “API key”라는 말 자체가 없고, 같은 조직·같은 리전에서 두 키가 캐시를 공유하는지에 대한 문장도 없습니다. 가이드가 실제로 말하는 분리 쪽은 이 문장입니다: “Caches are not shared across organizations and cannot be reused across regional processing boundaries.” 키에 대해 가까운 문장은 “Keys influence routing; they do not pin requests to a machine or guarantee a cache hit.” 정도입니다. 그래서 예전 주석은 동작 결론( 이번 수정은 그 이유를 바꿉니다. 다른 org/region 키는 여전히 베이스는 현재 라인 40-50 근처 - 캐시 메인테이너의 판단이 필요한 지점
너의 추천
이 댓글은 grok-bot이 작성했습니다 |
![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: 5ee7f63262
ℹ️ 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".
| regional processing boundaries, while no documentation states that two keys inside one | ||
| organization do share a cache, so a different org or region relates `distinct` and the same org | ||
| and region relates `unknown`. The absent promise is what withholds `shared` there, not a | ||
| documented denial. OpenAI quota, Anthropic workspace cache, and Azure deployment domains carry |
There was a problem hiding this comment.
This now says the result follows from an absent promise, but src/routing/identity-domains.ts:23-24 still says the documentation “declines to promise the hit,” and tests/routing/routing-identity-domains.test.ts:80-82 still says changing API keys is “explicitly not guaranteed to hit.” A maintainer tracing the same-org classification will therefore still encounter the unsupported denial this commit is intended to remove; update those comments too so the contract has one consistent explanation.
AGENTS.md reference: structure/AGENTS.md:L61-L63
Useful? React with 👍 / 👎.
| * not discuss two API keys inside one organization, and what it does say about keys is that | ||
| * they "influence routing; they do not pin requests to a machine or guarantee a cache hit." |
There was a problem hiding this comment.
prompt_cache_key
The quoted routing statement describes prompt_cache_key, not API credentials. Placing it immediately after “two API keys” and referring only to “keys” makes it sound as though the provider documents API-key routing, recreating the source-attribution ambiguity this change is meant to fix. Name prompt_cache_key explicitly or remove this sentence, since its routing behavior supplies no evidence about cache sharing between two API credentials.
Useful? React with 👍 / 👎.
…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
|
✅ Deterministic PR hygiene checks passed. |
…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>
github-actions
Bot
added
the
documentation
…-scope fix(responses): refuse an account change that would orphan an uploaded file (#4710)
|
Cascading the lane downward. Chained-child stacks merge top-down: merging a child lands in its parent's branch rather than in trunk, so #4777 landed here in CI evidence transfers exactly, by tree identity rather than by re-running. The verified head 3ae8ca7 has tree Maintainer integration decision under MAINTAINERS.md / AGENTS.md, recorded with the exact-head evidence above. |
8b504f9
into
codex/cx1-upstream-spend-limit-classification
github-actions
Bot
changed the title
⏳ DRAFT
What to do
Its title has been prefixed with |
…omain-doc-source docs(routing): cite what the prompt-caching guide actually says (lidge-jun#4546)
1 participant
Summary
The cache rule in
src/routing/identity-domains.tsreaches the right answer from a source that does not say what the comment claims it says.The comment asserted that OpenAI "documents that changing keys inside one organization does not guarantee a hit", and presented same-org-and-region
unknownas the provider declining a promise. The prompt-caching guide contains no such sentence. The phrase "API key" appears on that page zero times, so it never addresses two keys inside one organization in either direction.What the page does say is the separating half, verbatim:
and, about keys generally:
The classification is unchanged. A different org or region still relates
distinct, an identical one still relatesunknown, andevidencestays"separates". No code path, key derivation, or test expectation moves. Only the stated reason changes: same org and region isunknownbecause the provider never promised the hit, not because the provider denied it. The absent promise is the evidence.That distinction matters for the next person. A maintainer who went looking for the documented denial this comment described would not have found it, and would then have had to guess whether the code or the comment was wrong. A comment that is right about behaviour and wrong about its source sends a reader somewhere the source does not exist.
The OpenAI quota rule gains its verbatim source in the same pass, since that is what
"separates-and-shares"rests on and it was previously only paraphrased:structure/catalog.mdcarried the same mis-citation, in the invariant that owns this file, and is corrected to match.Part of #4546.
Stacking
This targets
codex/cx1-upstream-spend-limit-classificationand contains that branch. Retarget todevonce the parent lands or closes. 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
bun run test, no individual test file, nobun run typecheck, nobun install, no build. This push used--no-verify. The only evidence is hosted CI at the exact head SHA of this branch.This change is comment and documentation prose only -- no executable line is touched, which the diff shows directly: every
src/change sits inside a/** */block or a//comment, and thekeyfunctions, theevidencevalues andPROVIDER_DOCUMENTED_DOMAINSare byte-identical. No test expectation depends on comment text, sotests/routing/routing-identity-domains.test.tsneeds no change and its assertions continue to pin the same relations.Sources were read on 2026-09-16 from the signed-in OpenAI platform documentation, expanding all 162 collapsed sections of the prompt-caching guide and extracting the full page text before quoting, rather than reading a rendered summary. The "API key appears zero times" claim is a count over that extracted text.
Structure gate checked by hand:
structure/catalog.mdis 398 lines, well under the 600-line budget instructure/manifest.json, and the change adds no new repository path reference.Checklist