Repository navigation
fix(codex): carry canonical subagent limits across modes (#7125) - #7128
Conversation
Read and write the canonical Codex thread limit consistently with supported aliases. Keep the subagent selector available across modes, normalize child-count units, and reject stale settings responses. Cover canonical settings, mode behavior, and response ordering with focused regressions and update the capacity documentation.
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
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 |
|
✅ Deterministic PR hygiene checks passed. |
|
Maintainer integration into
|
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. |
![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: 7208c11cdd
ℹ️ 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".
| export function threadLimitToChildCount(limit: number, v2Enabled: boolean): number { | ||
| return v2Enabled ? Math.max(1, limit - 1) : limit; |
There was a problem hiding this comment.
When global V2 stores maxConcurrentThreadsPerSession = 1, that value permits only the root thread and therefore represents zero children, but this clamp reports one child. The Models selector and folded summary consequently overstate the available capacity, and selecting the displayed value is treated as a no-op rather than changing storage to a total of 2. Preserve the actual zero-child value for display and handle its read-only or editable representation explicitly.
AGENTS.md reference: gui/AGENTS.md:L7-L10
Useful? React with 👍 / 👎.
| if (options.threadLimit !== undefined && (!Number.isSafeInteger(options.threadLimit) || options.threadLimit < 1)) { | ||
| return { ok: false, error: "thread limit must be a safe integer >= 1" }; |
There was a problem hiding this comment.
Now that this transition rejects non-safe integers, the management route still accepts values such as 9007199254740992 with Number.isInteger, and the GUI custom-limit path does the same before issuing its PUT. Those inputs therefore pass public validation and are reported as a 502 transition failure instead of the documented validation error; in V2, even a safe child count can become unsafe after adding the root slot. Validate the final backend-unit value with Number.isSafeInteger in both request entry points and return the normal 400/inline error.
Useful? React with 👍 / 👎.
2 participants
Summary
4bc46305ec63ed62b4f8db22b8f74a0b9460f612, preserving the original implementation and authorship. The fork disallows maintainer edits. Based on currentorigin/dev(7640a942b, including the fix(ci): remove the shared root causes of flaky Windows CI #7107 Windows fix).[agents].max_concurrent_threads_per_sessionwith legacy-alias compatibility, preserve child-count capacity across V1/base/hybrid/global-V2 modes, and prevent older settings responses from reverting a completed save. Keep the API's existing backend-specific units; the UI consistently edits child counts. Update the capacity contract and translated user guidance.test 3/4regression atcodex-config-write-lock.test.ts:530: an atomic V2-to-V1 toggle now writes the canonical key, while the old assertion still demandedmax_threads. Assert the parsed agents table, including absence of the old alias. The original aggregatecifailure was downstream of that test failure. Correct the API field name in both carried architecture documents. Conflict warnings now identify the canonical agents key and its legacy alias in the CLI and every GUI locale, with focused regressions.Co-authored-by: Epinephrine 27862058+luvs01@users.noreply.github.com
Verification
bun test tests/codex-integration/codex-agent-thread-limit.test.ts tests/codex-integration/codex-config-write-lock.test.ts tests/codex-integration/codex-v2-gate.test.ts tests/cli/cli-v2-runtime.test.ts tests/test-layout.test.ts tests/test-layout-tooling.test.ts: 360 pass, 0 fail.gui/, initialbun test tests/models: 166 pass, 0 fail across 19 files. After adding the conflict-warning regression,bun test tests/models-subagent-limit.test.tsx: 11 pass, 0 fail (the new warning test also failed against the previous copy).bun run lint,bun run lint:i18n, andbun run build: passed after the copy change. Finalbun test tests/cli/cli-v2-runtime.test.ts: 112 pass, 0 fail.bun test tests/codex-integration/codex-missing-home.test.ts: 24 pass, 0 fail. Counts represent separate runs, not a summed whole-suite claim.bun run typecheck,bun run structure:check,bun run privacy:scan,bun scripts/file-size-ratchet.ts, andgit diff --check: passed.docs-site/,bun install --frozen-lockfileandbun run build: passed, 577 pages and 81,224 internal links checked.{maxConcurrentThreadsPerSession:32}and displays 32. This does not attest a running native host's reload behavior.bun run test:changed(fresh, round 2, current head): 29,797 pass / 39 skip / 10 fail / 4 errors across 1,331 files. Every failure is attributed to a defect that reproduces on cleanorigin/dev(7640a942b) without this PR; none touches the limit code (no failing file references[agents]limits,transitionMultiAgentV2, or the Models limit UI):a secondary ensure parent preserves shared Grok, Codex and Claude agent bytestimes out at 30 s with child exit 143. A serialbun test --isolate tests/cli/sibling-home-client-sync.test.tsgives the same 32 pass / 1 fail / 1 error on both branch and dev.native open and non-readiness events cannot claim readiness(5 s timeouts).OCX_TEST_FULL_SUITE=1 bun test --isolate --parallel=4over the five previously failing files gives an identical 322 pass / 4 fail / 4 errors on dev and branch, repeated twice per tree, with the same four failing tests. The clean-dev fullbun run testalso hits them.service install state Codex SQLite home binding, 2service diagnosticsstale-path cases): these fail in under 1 ms becauseresolveServiceState()reads a foreign record from the legacyhomedir()/.opencodex/service-state.json. Under the wrapper, Bun'shomedir()stays at the run-wide sandbox shared by all workers, while each worker's preload movesprocess.envhomes to a nested sandbox, so a service-state write anywhere in the run leaks to later readers. The leaked record'scodexHomeis a nested preload sandbox (…/opencodex-test-*/tmp/opencodex-test-*/.codex). Deterministic repro on clean dev with a seeded legacy file under a tempHOME:service-sqlite-home2 pass / 3 fail andservice.test.ts208 pass / 3 fail (the two sameservice diagnosticscases plusrejects restore operations from a different CODEX_HOME); the unseeded control passes 5/5 and 211/211. Whether it fires depends on file ordering, so the clean-dev full run happened to pass these. This is a pre-existing harness isolation defect, out of scope for this carry.OCX_TEST_NO_QUEUE=1 bun test --isolate <file>),cli-access-audio-live(36/36),responses-canonical-nonstream(41/41),service-sqlite-home(5/5), andservice(211/211) pass on both branch and dev. Round 1's extra live-audio and buffered-response failures did not recur.bun run teston7640a942bfails the same sibling-home and live-audio tests, plus a dev-onlyconnected Desktop uses hub IDs across a cold restartfailure.agents.max_concurrent_threads_per_sessionwithmax_threadsas an alias (binary strings and themust be at least 1validator), so the canonical key is the right source of truth in v1/base/hybrid modes, and full V2 keepsfeatures.multi_agent_v2.max_concurrent_threads_per_session(total = children + 1). Legacy keys are rewritten only by an explicit limit save or a v2 transition. Config injection calls the transition only while the global v2 flag is on, a state codex-rs already refuses to boot with any[agents]limit present, so startup never silently renames a user'smax_threads. Conflicting alias values refuse a write unless an explicit limit resolves them. The GUI converts units only when the global flag is on, and the stale-GET guard covers both read and write generations. Tests touching these paths usecreateTempHome/tempCODEX_HOMEs and restore env; no real user config is touched.lane=alldispatch and PR CI on headc0a798c1ba8a1c050262ea99ae542aaea92a6340are still queued as of 2026-10-15 09:30 UTC. Only thechanges/select windows runnerjobs have run, behind a repository-wide hosted-runner backlog of 100+ queued runs with one job in progress. No test job has produced a result for this head, so the PR stays draft until hosted CI completes. No changes to CI policy, production locks, ACLs, provider configuration, or user homes.Screenshot is stored only on the separate
pr-assetsbranch and pinned by commit SHA.Checklist