refactor(codex): keyed turn routing, client-scoped rate limits, and resume subscription safety#101376
Merged
Conversation
openclaw-barnacle
Bot
added
extensions: codex
size: XL
maintainer
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7695a70370
ℹ️ 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".
Comment on lines
28
to
+30
| export function readCodexNotificationThreadId(record: JsonObject): string | undefined { | ||
| return readNestedTurnThreadId(record) ?? readString(record, "threadId"); | ||
| const thread = isJsonObject(record.thread) ? record.thread : undefined; | ||
| return readString(record, "threadId") ?? (thread ? readString(thread, "id") : undefined); |
Contributor
There was a problem hiding this comment.
{turn} terminal notifications
The current Codex app-server README documents turn/started and turn/completed notifications as params shaped like { turn } rather than carrying a top-level threadId, and this change also stops reading turn.threadId. With that wire shape readScope() has no thread id, so ClientTurnRouter.routeNotification() returns before terminal handling; normal turns then ignore turn/completed and wait until the idle/attempt timeout. Please keep a nested or otherwise keyed correlation path for {turn} events before dropping them.
Useful? React with 👍 / 👎.
Contributor
|
Codex review: needs real behavior proof before merge. Reviewed July 7, 2026, 2:21 AM ET / 06:21 UTC. Summary PR surface: Source +803, Tests +1148. Total +1951 across 48 files. Reproducibility: yes. at source level: current main has the global Codex rate-limit cache and direct resume adoption described by the linked issue, and the PR source shows the active-turn wait regression. I did not run a live multi-client Codex reproduction in this read-only review. Review metrics: none identified. Stored data model Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance:
Risk before merge
Maintainer options:
Next step before merge
Maintainer decision needed
Security Review findings
Review detailsBest possible solution: Restore exact activeTurnIds-based native-turn waiting, add redacted current-head live Codex app-server proof for rate-limit, resume, and routing paths, then land this PR only if maintainer review accepts the broad refactor. Do we have a high-confidence way to reproduce the issue? Yes at source level: current main has the global Codex rate-limit cache and direct resume adoption described by the linked issue, and the PR source shows the active-turn wait regression. I did not run a live multi-client Codex reproduction in this read-only review. Is this the best way to solve the issue? No for the current head: per-client runtime state and keyed routing are the right layer, but the active native-turn wait must preserve exact reported turn IDs before this is the best fix. Live behavior proof is still required before merge. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 3175d8e7423d. Label changesLabel changes:
Label justifications:
Evidence reviewedPR surface: Source +803, Tests +1148. Total +1951 across 48 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
Review history (1 earlier review cycle)
|
clawsweeper
Bot
added
rating: 🦪 silver shellfish
Replace the process-global rate-limit cache with a WeakMap keyed by the physical app-server client, tracking per-limitId revisions. Rolling account/rateLimits/updated notifications merge sparsely per the protocol contract (credits/individualLimit/planType survive nulls), and usage-limit errors only trust a snapshot for auth-profile blocking when the same client observed a primary update during the failing turn's startup. Fixes cross-client rate-limit bleed in usage-limit error messages. A new client-runtime module installs the account/chatgptAuthTokens/refresh handler and the rate-limit observer once per physical client, replacing per-start inline handlers in shared-client, run-attempt, and side-question.
…resume Move the thread/resume request out of thread-lifecycle into a dedicated thread-resume module that retires the exact physical client when resume acceptance is indeterminate: only a structured RPC rejection proves Codex holds no subscription, so any other failure abandons the client instead of returning it to the shared pool. Resume responses naming a different thread now fail closed (assertCodexThreadResumeSubscription), and the fresh-thread fallback requires a released subscription unless the resume was a proven RPC rejection.
…ent factory Delete the lazy positional-argument CodexAppServerClientFactory and use the options-object factory type exported from shared-client. Callers in run-attempt, compact, bounded-turn, provider-capabilities, and the web-search provider now default to getLeasedSharedCodexAppServerClient directly; the lazy indirection was ineffective because those modules already import shared-client statically.
Install one turn router per physical app-server client and replace the broad per-attempt notification/request fanout with explicit per-thread routes. Attempt startup reserves the thread route (before thread/resume on the resume path, so early notifications buffer instead of racing), run-attempt activates it with receive-time, queued, and request handlers, arms the route before turn/start, binds the accepted turn id to flush buffered traffic in wire order, and releases the route on cleanup. Requests for a pending turn wait for binding instead of being auto-declined, native turn completion waits use route state instead of scanning buffered notifications, and correlation readers now match the canonical v2 wire shapes only (top-level threadId, nested turn.id). The unscoped response-delta lease-count attribution and its client API are deleted along with the retired correlation predicates.
…tests SQLite bindings are keyed by session identity rather than the per-test temp dir, so earlier tests leaked resumable threads into fresh-start expectations. The old silent resume-failure fallback masked the leak; subscription safety surfaces it.
The loop reuses one session identity across iterations, so the previous iteration's thread would resume against a harness that cannot serve it.
steipete
force-pushed
the
claude/cranky-volhard-0eb701
branch
from
July 7, 2026 06:04
7695a70 to
22c295c
Compare
Contributor
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 22c295c9cb
ℹ️ 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 nativeTurnCompleted = await waitForActiveNativeTurnCompletion(activeNativeTurnIds); | ||
| const nativeTurnCompleted = await waitForActiveNativeTurnCompletion(); |
Contributor
There was a problem hiding this comment.
When thread/resume reports active native turns, this call now waits for any terminal notification on the thread instead of the specific thread.lifecycle.activeTurnIds that blocked startup. If the resumed thread has multiple in-progress native turns, or a stale/other turn completion arrives while one listed active turn is still running, turn/start can be sent too early and reintroduce the active-turn collision this wait is meant to avoid. Use the reported active turn ids (or the exact watchNativeTurnCompletion path) before treating the native turn as complete.
Useful? React with 👍 / 👎.
clawsweeper
Bot
added
rating: 🧂 unranked krab
Contributor
Author
|
Merged via squash.
|
github-actions Bot
pushed a commit
to Desicool/openclaw
that referenced
this pull request
Jul 7, 2026
…esume subscription safety (openclaw#101376) * feat(codex): scope app-server rate limits to the physical client Replace the process-global rate-limit cache with a WeakMap keyed by the physical app-server client, tracking per-limitId revisions. Rolling account/rateLimits/updated notifications merge sparsely per the protocol contract (credits/individualLimit/planType survive nulls), and usage-limit errors only trust a snapshot for auth-profile blocking when the same client observed a primary update during the failing turn's startup. Fixes cross-client rate-limit bleed in usage-limit error messages. A new client-runtime module installs the account/chatgptAuthTokens/refresh handler and the rate-limit observer once per physical client, replacing per-start inline handlers in shared-client, run-attempt, and side-question. * refactor(codex): split thread/resume subscription safety into thread-resume Move the thread/resume request out of thread-lifecycle into a dedicated thread-resume module that retires the exact physical client when resume acceptance is indeterminate: only a structured RPC rejection proves Codex holds no subscription, so any other failure abandons the client instead of returning it to the shared pool. Resume responses naming a different thread now fail closed (assertCodexThreadResumeSubscription), and the fresh-thread fallback requires a released subscription unless the resume was a proven RPC rejection. * refactor(codex): replace client-factory positional DI with shared-client factory Delete the lazy positional-argument CodexAppServerClientFactory and use the options-object factory type exported from shared-client. Callers in run-attempt, compact, bounded-turn, provider-capabilities, and the web-search provider now default to getLeasedSharedCodexAppServerClient directly; the lazy indirection was ineffective because those modules already import shared-client statically. * feat(codex): route app-server turn traffic through a keyed turn router Install one turn router per physical app-server client and replace the broad per-attempt notification/request fanout with explicit per-thread routes. Attempt startup reserves the thread route (before thread/resume on the resume path, so early notifications buffer instead of racing), run-attempt activates it with receive-time, queued, and request handlers, arms the route before turn/start, binds the accepted turn id to flush buffered traffic in wire order, and releases the route on cleanup. Requests for a pending turn wait for binding instead of being auto-declined, native turn completion waits use route state instead of scanning buffered notifications, and correlation readers now match the canonical v2 wire shapes only (top-level threadId, nested turn.id). The unscoped response-delta lease-count attribution and its client API are deleted along with the retired correlation predicates. * test(codex): reset the shared binding store between thread-lifecycle tests SQLite bindings are keyed by session identity rather than the per-test temp dir, so earlier tests leaked resumable threads into fresh-start expectations. The old silent resume-failure fallback masked the leak; subscription safety surfaces it. * test(codex): reset the binding store between delivery-hint iterations The loop reuses one session identity across iterations, so the previous iteration's thread would resume against a harness that cannot serve it.
This was referenced Jul 7, 2026
sheyanmin
pushed a commit
to sheyanmin/openclaw
that referenced
this pull request
Jul 8, 2026
…esume subscription safety (openclaw#101376) * feat(codex): scope app-server rate limits to the physical client Replace the process-global rate-limit cache with a WeakMap keyed by the physical app-server client, tracking per-limitId revisions. Rolling account/rateLimits/updated notifications merge sparsely per the protocol contract (credits/individualLimit/planType survive nulls), and usage-limit errors only trust a snapshot for auth-profile blocking when the same client observed a primary update during the failing turn's startup. Fixes cross-client rate-limit bleed in usage-limit error messages. A new client-runtime module installs the account/chatgptAuthTokens/refresh handler and the rate-limit observer once per physical client, replacing per-start inline handlers in shared-client, run-attempt, and side-question. * refactor(codex): split thread/resume subscription safety into thread-resume Move the thread/resume request out of thread-lifecycle into a dedicated thread-resume module that retires the exact physical client when resume acceptance is indeterminate: only a structured RPC rejection proves Codex holds no subscription, so any other failure abandons the client instead of returning it to the shared pool. Resume responses naming a different thread now fail closed (assertCodexThreadResumeSubscription), and the fresh-thread fallback requires a released subscription unless the resume was a proven RPC rejection. * refactor(codex): replace client-factory positional DI with shared-client factory Delete the lazy positional-argument CodexAppServerClientFactory and use the options-object factory type exported from shared-client. Callers in run-attempt, compact, bounded-turn, provider-capabilities, and the web-search provider now default to getLeasedSharedCodexAppServerClient directly; the lazy indirection was ineffective because those modules already import shared-client statically. * feat(codex): route app-server turn traffic through a keyed turn router Install one turn router per physical app-server client and replace the broad per-attempt notification/request fanout with explicit per-thread routes. Attempt startup reserves the thread route (before thread/resume on the resume path, so early notifications buffer instead of racing), run-attempt activates it with receive-time, queued, and request handlers, arms the route before turn/start, binds the accepted turn id to flush buffered traffic in wire order, and releases the route on cleanup. Requests for a pending turn wait for binding instead of being auto-declined, native turn completion waits use route state instead of scanning buffered notifications, and correlation readers now match the canonical v2 wire shapes only (top-level threadId, nested turn.id). The unscoped response-delta lease-count attribution and its client API are deleted along with the retired correlation predicates. * test(codex): reset the shared binding store between thread-lifecycle tests SQLite bindings are keyed by session identity rather than the per-test temp dir, so earlier tests leaked resumable threads into fresh-start expectations. The old silent resume-failure fallback masked the leak; subscription safety surfaces it. * test(codex): reset the binding store between delivery-hint iterations The loop reuses one session identity across iterations, so the previous iteration's thread would resume against a harness that cannot serve it.
giodl73-repo
pushed a commit
to giodl73-repo/openclaw
that referenced
this pull request
Jul 8, 2026
…esume subscription safety (openclaw#101376) * feat(codex): scope app-server rate limits to the physical client Replace the process-global rate-limit cache with a WeakMap keyed by the physical app-server client, tracking per-limitId revisions. Rolling account/rateLimits/updated notifications merge sparsely per the protocol contract (credits/individualLimit/planType survive nulls), and usage-limit errors only trust a snapshot for auth-profile blocking when the same client observed a primary update during the failing turn's startup. Fixes cross-client rate-limit bleed in usage-limit error messages. A new client-runtime module installs the account/chatgptAuthTokens/refresh handler and the rate-limit observer once per physical client, replacing per-start inline handlers in shared-client, run-attempt, and side-question. * refactor(codex): split thread/resume subscription safety into thread-resume Move the thread/resume request out of thread-lifecycle into a dedicated thread-resume module that retires the exact physical client when resume acceptance is indeterminate: only a structured RPC rejection proves Codex holds no subscription, so any other failure abandons the client instead of returning it to the shared pool. Resume responses naming a different thread now fail closed (assertCodexThreadResumeSubscription), and the fresh-thread fallback requires a released subscription unless the resume was a proven RPC rejection. * refactor(codex): replace client-factory positional DI with shared-client factory Delete the lazy positional-argument CodexAppServerClientFactory and use the options-object factory type exported from shared-client. Callers in run-attempt, compact, bounded-turn, provider-capabilities, and the web-search provider now default to getLeasedSharedCodexAppServerClient directly; the lazy indirection was ineffective because those modules already import shared-client statically. * feat(codex): route app-server turn traffic through a keyed turn router Install one turn router per physical app-server client and replace the broad per-attempt notification/request fanout with explicit per-thread routes. Attempt startup reserves the thread route (before thread/resume on the resume path, so early notifications buffer instead of racing), run-attempt activates it with receive-time, queued, and request handlers, arms the route before turn/start, binds the accepted turn id to flush buffered traffic in wire order, and releases the route on cleanup. Requests for a pending turn wait for binding instead of being auto-declined, native turn completion waits use route state instead of scanning buffered notifications, and correlation readers now match the canonical v2 wire shapes only (top-level threadId, nested turn.id). The unscoped response-delta lease-count attribution and its client API are deleted along with the retired correlation predicates. * test(codex): reset the shared binding store between thread-lifecycle tests SQLite bindings are keyed by session identity rather than the per-test temp dir, so earlier tests leaked resumable threads into fresh-start expectations. The old silent resume-failure fallback masked the leak; subscription safety surfaces it. * test(codex): reset the binding store between delivery-hint iterations The loop reuses one session identity across iterations, so the previous iteration's thread would resume against a harness that cannot serve it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #101338
Related: #93313
What Problem This Solves
Fixes an issue where a Codex usage-limit failure could report another account's rate-limit reset time and block the wrong auth profile: the rate-limit cache was process-global, so snapshots observed by one physical app-server client leaked into usage-limit errors on a different client. It also replaces the broad per-attempt notification/request fanout that made every attempt see and re-filter all app-server traffic, let
thread/resumeadopt mismatched or indeterminate subscriptions on pooled clients, and reinstalled auth-refresh handlers per attempt/side-question instead of per client.Why This Change Was Made
Re-implements the turn-routing and client-runtime-ownership track of #93313 on current
main(that branch is thousands of commits behind and cannot be rebased). One turn router per physical client owns explicit per-thread routes with a reserve → arm → bind lifecycle: startup reserves the route beforethread/resumeso early notifications buffer instead of racing,turn/startarms the route, and binding the accepted turn id flushes buffered traffic in wire order. Rate limits move to aWeakMapkeyed by the physical client with per-limitIdrevisions, merging rollingaccount/rateLimits/updatedpayloads per the protocol's sparse-update contract; usage-limit errors only trust a snapshot for profile blocking when the same client observed a primary update during the failing turn's startup.thread/resumemoves into a dedicated module that retires the exact client when acceptance is indeterminate (only a structured RPC rejection proves no subscription). The positional-argclient-factory.tsindirection is deleted. Out of scope, tracked in #101338: the #93313 native-subagent-monitor status-revision rework and its compact generalization (main has since shipped a newer compaction completion-watch design).User Impact
Usage-limit errors now report the reset time for the account that actually hit the limit, and auth-profile blocking can no longer be triggered by another client's rate-limit traffic. Turn traffic on shared Codex app-server processes is routed per thread/turn, and a resume that Codex did not provably accept can no longer return a subscribed client to the shared pool. No config or API surface changes.
Evidence
@openai/codex0.142.5 generated schemas and upstreamcodex-rssource (sparse rolling rate-limit updates,willRetrycamelCase, top-levelthreadIdon turn-scoped notifications,thread/unsubscribestatus semantics).extensions/codexsuite green on Blacksmith Testbox (95 files / 1985 tests; final rerun in PR checks).pnpm check:changedgreen on Blacksmith Testbox (typecheck extensions + tests, oxlint, guards): Actions run https://github.com/openclaw/openclaw/actions/runs/28841817056.will_retryalias removal) are fixed with regression tests. New regression coverage: cross-limit rate-limit trust, resume echo/mismatch retirement, rejected-resume unsubscribe fallback, keyed-router unit suite (940-lineturn-router.test.ts).