feat(codex): give V2 threads real lineage and place a child on its parent's serving account (#4546) - #4640
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. |
|
✅ Deterministic PR hygiene checks passed. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (12)
📝 WalkthroughWalkthroughCodex V2 affinity now uses opaque per-thread conversation keys. Authenticated lineage is stored with TTL and LRU bounds. Initial child placement can follow a parent or sibling account, while response previews use the same credential scope as final resolution. ChangesCodex lineage-aware pool placement
Estimated code review effort: 4 (Complex) | ~60 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant Responses
participant AuthContext
participant Lineage
participant Routing
Responses->>AuthContext: derive credential-domain headers and Pool eligibility
AuthContext->>Lineage: preview or record thread lineage
AuthContext->>Routing: resolve account with lineage
Routing->>Lineage: inspect parent and sibling lineage
Lineage-->>Routing: return serving-account hint
Routing-->>Responses: return preview or selected account
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![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: 50dba9cba6
ℹ️ 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".
| const previewAuthHeaders = codexRouteCredentialDomainHeaders( | ||
| req, | ||
| route, | ||
| options, | ||
| credentialDomainWasRewritten, |
There was a problem hiding this comment.
When a parent-only request enters a policy or other credential-domain rewrite that strips Authorization, previewAuthHeaders selects the local lineage scope, but poolAffinityKey was already derived from req.headers. Because codexConversationIdentity consults the scoped parent record to resolve parent-only turns, the preview can query HMAC(parent,parent) while final auth reuses the recorded HMAC(session,parent) binding, causing subagent fallback to evaluate a cold account instead of the account that will actually serve the request. Derive the preview affinity key from previewAuthHeaders as well.
Useful? React with 👍 / 👎.
| const poolLineage = previewCodexPoolLineage(previewAuthHeaders, options.codexAuthPolicy ?? config, { | ||
| accountId: route.codexAccountId, | ||
| modelId: route.modelId, | ||
| admission: options.admission, |
There was a problem hiding this comment.
When a thread spawn starts on an account-qualified route and fallback rewrites it to an unqualified Pool route, this initial-route calculation returns no lineage because route.codexAccountId is fixed, and the same poolLineage is then reused for every fallback candidate. Final auth resolves against the rewritten unqualified route and does record/use lineage, so fallback can judge model health or quota against a cold account while the request is ultimately placed on its family's account. Compute eligibility and lineage for each candidate route, including the post-recovery fallback path, rather than freezing the initial route's result.
Useful? React with 👍 / 👎.
| `x-codex-parent-thread-id` rides that parent's own lane as `app:HMAC(parent, parent)`: one parent | ||
| id still maps to exactly one lane, and no caller-supplied identifier reaches Pool state. Which |
There was a problem hiding this comment.
When the parent has already been recorded and its session-id differs from its thread-id, codexConversationIdentity deliberately returns the recorded HMAC(session,parent) key for a parent-only request; it falls back to HMAC(parent,parent) only when no scoped record exists. This authoritative contract currently states that every such request uses the fallback, contradicting both the implementation and the added regression test, so update it to describe the recorded-key precedence.
AGENTS.md reference: structure/AGENTS.md:L9-L10
Useful? React with 👍 / 👎.
| @@ -0,0 +1,458 @@ | |||
| /** | |||
There was a problem hiding this comment.
This adds a new src/codex/ module and changes src/server/, but the commit updates only structure/providers/openai-tiers.md; structure/INDEX.md maps src/codex/ to eight documents and src/server/ to fourteen, leaving the other mapped owners untouched. Update every mapped document in this change, or correct the manifest ownership if those documents do not actually describe these areas.
AGENTS.md reference: structure/AGENTS.md:L49-L50
Useful? React with 👍 / 👎.
리뷰 · 우선순위 69 / 80이 PR은 스택 문서의 wpd(V2 lineage placement) 입니다. 문서 표에서는 wpf 다음·wpa 앞(4번)이었는데, 실제 브랜치는 문제는 plain합니다.
라인 src/codex/lineage.ts (프로세스 로컬·HMAC 키) - 재시작 후 혈통이 사라지는 것은 의도입니다. 다만 롤링 배포 중 두 프로세스가 같은 대화를 나눠 보면 배치 힌트가 갈라집니다. 단일 인스턴스 전제인지 운영 노트에 남기세요. 라인 스택 순서 - 문서는 wpd가 wpa/wpb와 독립(계약 모듈)이라고 했지만, 실제 base는 wph입니다. 독립이 맞다면 경로 CI - test 1/4 실패·다수 pending. 앞 스택 빨간불과 겹칩니다. wpd 고유 실패인지 분리해 보세요. 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
e7dc6f9 to
4ecb95f
Compare
50dba9c to
7f7c4fc
Compare
4ecb95f to
67c4115
Compare
7f7c4fc to
b85b0bd
Compare
67c4115 to
a0918cd
Compare
b85b0bd to
bd2a4d4
Compare
a0918cd to
34614c7
Compare
bd2a4d4 to
3711e49
Compare
b467d4f to
f1a140c
Compare
34614c7 to
fe8917d
Compare
975d177 to
635f631
Compare
29c9aa3 to
6966470
Compare
…rent's serving account (#4546) Part of the stacked delivery closing the remaining OCX-4546 cost-guard scope. Verification posture: local suite, typecheck, install and build NOT run by explicit instruction. Hosted CI at the exact final head is the only proof. Pushed with --no-verify.
…legacy affinity key (#4546) Review findings on the V2 lineage layer: a parent-only request keyed HMAC(parent,parent), which equals the root key only when session-id equals thread-id, so a real root followed by a parent-only turn started cold; a binding made under the old raw-parent key was never probed, so a live conversation was silently cold-rebound across an in-process code swap; preview derived lineage from raw headers before final auth decided whether Pool state was permitted; and current-serving-account ignored model-detour affinity. Verification posture: local suite, typecheck, install and build NOT run by explicit instruction. Hosted CI at the exact final head is the only proof. Pushed with --no-verify.
…ent relatively (#4546) Hosted CI failed three lineage rows. The real defect: a parent-only turn resolves its key through the recorded lineage, which is TTL-bounded, but codexPoolAffinityKey read Date.now() internally - so any caller on a fixed clock saw a live record as expired and fell back to HMAC(parent,parent), the key the parent never bound under. The function now takes the clock like everything else on this path. The other two rows asserted exact account names derived from quota-strategy ordering the author reasoned through but could not observe. They now assert what this layer actually promises: the child binds to whatever account is serving its parent at placement time, an already-bound child is untouched when the parent later moves, and a new child reads the parent's current account instead of its sibling's. Verification posture: local suite, typecheck, install and build NOT run by explicit instruction. Hosted CI at the exact final head is the only proof.
… a fixture account (#4546) Two more rows encoded quota-fixture outcomes as invariants. Where the parent lands after its own quota refusal is the strategy's decision and may legitimately be the account the child already holds, so nothing is asserted about that destination. The orphan row now asserts that the child follows its SIBLING's actual placement, which is the reachable half of the family when the parent is ineligible, instead of naming an account the fixture happened to produce. Verification posture: local suite, typecheck, install and build NOT run by explicit instruction. Hosted CI at the exact final head is the only proof.
…ity key (#4546) The suite asserted the binding under the RAW parent thread id, which is the keying this layer deliberately removes: a thread now keys as itself through an opaque HMAC and the parent header is a first-placement hint. The read-back uses the derived key, so the assertion still proves the replayed account stays selectable on the next request without pinning the old raw-parent key. Verification posture: local suite, typecheck, install and build NOT run by explicit instruction. Hosted CI at the exact final head is the only proof.
The #2509 oracle asserts both fallback preview sites forward the model-eligible account set, but its regex required modelId to be the final argument. This layer appends the resolved pool lineage so preview and final resolution agree on a child's first turn, which is a new trailing argument rather than a dropped eligible set. The pattern now allows anything after modelId and keeps the guarantee it exists for. Verification posture: local suite, typecheck, install and build NOT run by explicit instruction. Hosted CI at the exact final head is the only proof.
38df147 to
cc70912
Compare
…rent's serving account (lidge-jun#4546) (lidge-jun#4640) * feat(codex): give V2 threads real lineage and place a child on its parent's serving account (lidge-jun#4546) Part of the stacked delivery closing the remaining OCX-4546 cost-guard scope. Verification posture: local suite, typecheck, install and build NOT run by explicit instruction. Hosted CI at the exact final head is the only proof. Pushed with --no-verify. * fix(codex): resolve a parent-only turn through lineage and adopt the legacy affinity key (lidge-jun#4546) Review findings on the V2 lineage layer: a parent-only request keyed HMAC(parent,parent), which equals the root key only when session-id equals thread-id, so a real root followed by a parent-only turn started cold; a binding made under the old raw-parent key was never probed, so a live conversation was silently cold-rebound across an in-process code swap; preview derived lineage from raw headers before final auth decided whether Pool state was permitted; and current-serving-account ignored model-detour affinity. Verification posture: local suite, typecheck, install and build NOT run by explicit instruction. Hosted CI at the exact final head is the only proof. Pushed with --no-verify. * fix(codex): thread the clock into the affinity key, and assert placement relatively (lidge-jun#4546) Hosted CI failed three lineage rows. The real defect: a parent-only turn resolves its key through the recorded lineage, which is TTL-bounded, but codexPoolAffinityKey read Date.now() internally - so any caller on a fixed clock saw a live record as expired and fell back to HMAC(parent,parent), the key the parent never bound under. The function now takes the clock like everything else on this path. The other two rows asserted exact account names derived from quota-strategy ordering the author reasoned through but could not observe. They now assert what this layer actually promises: the child binds to whatever account is serving its parent at placement time, an already-bound child is untouched when the parent later moves, and a new child reads the parent's current account instead of its sibling's. Verification posture: local suite, typecheck, install and build NOT run by explicit instruction. Hosted CI at the exact final head is the only proof. * test(codex): assert the sibling fallback relative to the sibling, not a fixture account (lidge-jun#4546) Two more rows encoded quota-fixture outcomes as invariants. Where the parent lands after its own quota refusal is the strategy's decision and may legitimately be the account the child already holds, so nothing is asserted about that destination. The orphan row now asserts that the child follows its SIBLING's actual placement, which is the reachable half of the family when the parent is ineligible, instead of naming an account the fixture happened to produce. Verification posture: local suite, typecheck, install and build NOT run by explicit instruction. Hosted CI at the exact final head is the only proof. * test(responses): read the pool binding back through the derived affinity key (lidge-jun#4546) The suite asserted the binding under the RAW parent thread id, which is the keying this layer deliberately removes: a thread now keys as itself through an opaque HMAC and the parent header is a first-placement hint. The read-back uses the derived key, so the assertion still proves the replayed account stays selectable on the next request without pinning the old raw-parent key. Verification posture: local suite, typecheck, install and build NOT run by explicit instruction. Hosted CI at the exact final head is the only proof. * test(routing): stop pinning modelId as the last preview argument (lidge-jun#4546) The lidge-jun#2509 oracle asserts both fallback preview sites forward the model-eligible account set, but its regex required modelId to be the final argument. This layer appends the resolved pool lineage so preview and final resolution agree on a child's first turn, which is a new trailing argument rather than a dropped eligible set. The pattern now allows anything after modelId and keeps the guarantee it exists for. Verification posture: local suite, typecheck, install and build NOT run by explicit instruction. Hosted CI at the exact final head is the only proof.
1 participant
Summary
Codex V2 threads now carry a real parent/child/grandchild relation, and a newly spawned child starts on the account that is actually serving its parent instead of cold.
codexPoolAffinityKeypreferredx-codex-parent-thread-id, so every child of one parent bound under the raw parent id: one shared entry, unrelated to the root's ownapp:HMAC(session, thread)binding. No child could hold a binding of its own, a grandchild keyed on something nobody had ever bound, and a newly spawned child started with a cold prefix while its parent was being served warm somewhere else.A request carrying
thread-idnow keys asapp:HMAC(session-id ?? parent, thread-id). The set of requests that bind at all is deliberately unchanged — a barethread-idwith no session and no parent still has no family anchor and stays unbound — and only the value moves, for requests naming a parent.src/codex/lineage.tsrecords, per authenticated scope, each thread's conversation key, its immediate parent and its transitive root, so a grandchild resolves to its root rather than to its parent. It is bounded in both dimensions, by idle TTL plus an LRU cap on records per scope and on scopes, and is never keyed across authentication scopes.Placement, and only placement, changes: a thread that has never bound starts on the account currently serving its parent — a live transient detour included, rather than the parent's stale home — then on a compatible sibling's, then on ordinary cold placement. The child holds an ordinary binding of its own, and the hint does not move the shared active-account cursor. The asymmetry is pinned: a parent bound to A but served by B places a new child on B, and when the parent later moves to C, the child already progressing on B is untouched while the next new child starts on C.
Four review findings are fixed on top of that. A parent-only turn now resolves through the existing scoped parent record instead of re-deriving
HMAC(parent, parent), which equals the root key only whensession-idhappens to equalthread-id. A lineage miss reads the legacy raw-parent key once, adopts its binding and retires the entry, so a conversation live across an in-process code swap is not silently cold-rebound — the exact defect this unit exists to prevent. Preview derives lineage from the same transformed auth headers and eligibility predicateresolveCodexAuthContextuses, so it cannot follow a family binding that final auth deliberately refuses to create. And "currently serving" now inspects the compatible model-detour entry before the ordinary binding.Stacked on #4639.
Verification
Not run, by explicit instruction: the local suite,
bun run typecheck,bun install, and any build. The only proof for this unit is hosted CI at the exact final head SHA; this push used--no-verify.New coverage in
tests/codex-integration/codex-lineage-placement.test.ts: twelve tests covering the unchanged unbound set, transitive root resolution inside one auth scope, bounds in both dimensions, a child starting on the parent's account under its own key, following the account actually serving the parent, the parent-move asymmetry, sibling placement, cold fallback, a parent-only turn continuing the parent's conversation with or without a session id, adoption of a binding left under the old raw-parent key, following the parent's model detour, and preview reading the family only for a request that may own Pool state.tests/codex-integration/codex-auth-context.test.tshas its one raw-value assertion rewritten to the new contract.Known open: the lineage-backed worker/interactive answer is exported but not consumed — admission still classifies from headers alone. The cost-attribution root lookup is exported and nothing spends against it yet.
Checklist
Summary by CodeRabbit
New Features
Documentation