Skip to content

fix(codex): honor canonical subagent limits across modes - #7125

Closed
luvs01 wants to merge 2 commits into
lidge-jun:devfrom
luvs01:codex/agent-limit-canonical-20261015
Closed

luvs01 wants to merge 2 commits into
lidge-jun:devfrom
luvs01:codex/agent-limit-canonical-20261015

Conversation

@luvs01

@luvs01 luvs01 commented Oct 15, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

  • Fix the mismatch between Codex's canonical max_concurrent_threads_per_session setting and OpenCodex's legacy thread-limit reader and writer. With a canonical value of 16, the old reader misses the value and saving 6 adds a legacy key instead of updating the canonical setting. Codex keeps using 16, reports duplicate configuration, and the UI incorrectly presents 6 as a successful save. Read and write the canonical setting consistently and reconcile supported aliases so the displayed value matches the effective setting.
  • Keep the sub-agent limit in Models → Model settings, including V1 and hybrid mode. Display the number of child agents consistently: V1 and hybrid values are already child counts; full V2 includes the main agent, so display total - 1 and save children + 1 only in full V2.
  • Prevent an older settings GET, including a late error or missing response, from overwriting a newer GET or successful settings write. Check both read and write generations before applying state.
  • Document the mode and capacity contract. Split the structure documentation to meet the existing file budget while preserving section anchors and registering the new page in the structure manifest and generated index.

Verification

Signed publication HEAD: 668e07ac6bcaa7482494e115e092dc1d80fdc32a; sole parent: 4bc46305ec63ed62b4f8db22b8f74a0b9460f612; tree: 9afd584475e86590dd18ee4e6fb2dc14115b6d83. The complete patch from base 7640a942b35044c974e534b84d5a8049f4a8ae9a is 36 files, 118,431 bytes, SHA256 2b812ff92d391d9265f113fc391ab732928e4d53a4e77102de62cd335e05fd9b. The follow-up is exactly one test file, 3 added lines and 1 removed line, matching the reviewed patch SHA256 e37e98ca07d6181f79fee416855ebf479074f0924368e501a12e736d12229594; production source is unchanged. GitHub signature verification is valid.

Integrated static validation for the original 35-file candidate: scripts/privacy-scan.ts, scripts/file-size-ratchet.ts, scripts/structure-ssot.ts, and git diff --cached --check against the base all exited 0. Final source typecheck with Node 24 also passed. The signed tree, staged source, and working source were checked for equality; no screenshot or temporary artifacts are included in the code commit.

CI follow-up and current local validation:

  • Previous-HEAD run 38696002038 completed with test shards 1/4, 2/4 and 4/4 passing; test 3/4 failed only on the old serialized max_threads = 63 assertion. The follow-up parses [agents], requires canonical max_concurrent_threads_per_session = 63, and rejects the legacy alias. The atomic rename callback and {ok:true, changed:true, threadLimit:63} assertion are retained; production ACL, write-lock and witness guards are unchanged.
  • RED reproduced the old assertion failure. The corrected assertion passed within the actual Windows combined run. Canonical sibling tests 15 passed and existing V2 gate tests 132 passed; these separate scopes are not added to earlier counts.
  • The Windows combined run across 14 files exited 1: 421 passed, 12 skipped, 5 failed, 2,422 assertions. Two subprocess cases returned null exit codes after five seconds, and three alias-staging cases observed false instead of true. Those unchanged tests passed on the previous HEAD in Ubuntu CI. A subsequent native Windows Bun 1.4.3 comparison ran the same five-name --isolate selection once against base 7640a942 and the candidate with the identical frozen assertion correction: both returned 3 passed, 2 failed. Only the two subprocess deadline failures are thereby confirmed as baseline failures under that command; their startup/ACL/lock cause remains UNVERIFIED. All three staging cases passed in both comparison runs, so the earlier combined-run staging failures remain UNRESOLVED, neither established baseline failures nor established feature regressions. The comparison did not control exclusive machine load, and no timeout, assertion, or behavior guard was weakened. These failing runs are not a full-suite pass.
  • Scoped privacy, file-size and diff checks passed for the test-only follow-up. Root production typecheck was not repeated because production source is unchanged. Hosted checks for the new signed HEAD remain pending; no old run was retried.

Completed backend and layout validation for the reviewed backend candidate:

  • Final backend combined regressions: 277 passed, 0 failed, 1,355 assertions across 6 files. Test-layout regressions: 27 passed. These are separate validation scopes and are not added to the GUI test count.
  • The configuration-write fixture now uses separate sibling home directories to avoid background configuration preparation changing destination metadata during the test. Existing write guards remain intact; production ACL and write-lock code are unchanged. Nested-home deployment behavior is not fixed by this fixture correction.
  • A fresh independent review of the final backend candidate passed its P2 check. This review does not replace maintainer/code-owner review or integration validation.

Completed GUI validation for the reviewed GUI candidate, from gui/:

  • bun test tests/models: 166 passed across 19 files, including four settings-response race regressions that failed before the fix.
  • bun run lint, bun run lint:i18n, and bun run build (tsc -b and Vite): exit 0.
  • The earlier 30 i18n tests were not rerun because the subsequent race fix did not change the translations. They are separate prior evidence and are not added to the 166-test result.
  • Headless mocked-API DOM verification: 8/8 assertions passed. With global V2 disabled, mode V2 selected, and the native-V1 pin enabled, the hybrid selector displays an editable 16; selecting 32 sends 32 in child units and resetting sends 16. A 1360 × 960 PNG was captured (197,595 bytes, SHA256 af3f1c163a5bcc623ba6f45458f1acae973a39fcdd7833c4f545d75d7a21737d). This verifies the fixture UI and request payloads, not a live native host or hosted capacity reload.

Hybrid mode sub-agent limit showing an editable child count of 16

The screenshot uses synthetic API fixtures and is stored on the separate pr-assets branch. Its URL is pinned to the asset commit; the downloaded bytes match the captured PNG's SHA256 above.

Completed documentation validation:

  • Node 24.18.0, node --preserve-symlinks --preserve-symlinks-main ./node_modules/astro/bin/astro.mjs build, from docs-site/: exit 0, 577 pages built and 81,224 internal links checked.
  • Generated English and Korean HTML was checked for Models → Model settings, child-count units, hybrid mode with the global V2 flag disabled, and the new-session and host-restart conditions.
  • scripts/structure-ssot.ts --fix, the subsequent read-only structure check, and scoped git diff --check: exit 0. The structure split resolves the document file-budget violation; this is not a claim that every final-candidate repository check has passed.

Pending validation and publication evidence:

  • The full suite and bun run test:changed were not run. Under the documented focused-test exception, the inherited merge-base selection expands to 148 changed files and disproportionate import-graph coverage while validation worktrees share finite resources. The focused regressions, typecheck, builds, and static gates above cover the changed behavior; broader coverage remains with hosted CI. No aggregate whole-repository test pass is claimed.
  • Hosted CI on the signed publication HEAD is pending. Earlier-head checks are not evidence for this HEAD.
  • Nested-home deployment behavior remains unresolved. Live native-host and hosted capacity reload behavior have not been verified.
  • Maintainer/code-owner review of canonical-key migration, alias compatibility, mode-specific units, and the settings race handling remains pending. Keep the PR in Draft.

Checklist

  • Scope stays focused and avoids unrelated cleanup.
  • Docs or release notes were updated when needed.
  • Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults.
  • The final integrated candidate's required validation is recorded.
  • The final patch hash and commit HEAD are recorded, and the required GUI screenshot is attached.
  • Maintainer/code-owner review is complete.
  • My PR is ready for review.

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.
@coderabbitai

coderabbitai Bot commented Oct 15, 2026

Copy link
Copy Markdown
Contributor

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

  • Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

Copy link
Copy Markdown
Contributor

✅ Deterministic PR hygiene checks passed.

@github-actions github-actions Bot added the bug Something isn't working label Oct 15, 2026
@lidge-jun

Copy link
Copy Markdown
Owner

Carrying this implementation in #7128 because this fork disables maintainer edits. The canonical source design is sound and the contributor commit is preserved, with Co-authored-by: Epinephrine <27862058+luvs01@users.noreply.github.com> included in the carry commit and PR description.

The failed test 3/4 was tests/codex-integration/codex-config-write-lock.test.ts:530: it still expected max_threads = 63 after an atomic V2-to-V1 feature toggle. This implementation correctly writes max_concurrent_threads_per_session = 63. Reproduced locally (75/75 on dev; 74/75 with your commit), then corrected the oracle to compare the parsed [agents] table so it also excludes a stale legacy alias. The aggregate ci failure follows that one failed shard.

The carry currently passes 360 focused backend/layout tests, 166 Models tests, typecheck, GUI lint/build/i18n, docs build, structure, privacy and the size ratchet. Linux/macOS/Windows lane=all coverage is dispatched at https://github.com/lidge-jun/opencodex/actions/runs/38698076771; the carry remains draft while validation finishes. No production lock, ACL, workflow or dependency changes were needed.

Parse the resulting agents configuration and assert the canonical child limit and absence of the legacy alias. Retain the atomic rename callback and successful transition assertions.
@lidge-jun

Copy link
Copy Markdown
Owner

Superseded by the in-repository carry #7128, which preserves your implementation commit and co-author credit. This closes the duplicate delivery path because maintainer edits are disabled here; it is not a rejection of the canonical-limit design.

The carry fixes the stale write-lock assertion, corrects the architecture API field name, and identifies both canonical and legacy keys in conflict diagnostics. Focused regressions and static/build gates pass. Broad local coverage has separately documented baseline and still-unclassified failures, so the carry remains draft. Exact-head Linux/macOS/Windows CI is dispatched at https://github.com/lidge-jun/opencodex/actions/runs/38699018462 and is still queued; no green claim or merge has been made.

@lidge-jun lidge-jun closed this Oct 15, 2026
lidge-jun added a commit that referenced this pull request Oct 15, 2026
* fix(codex): honor canonical subagent limits across modes

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.

* test(codex): require canonical limit after atomic feature toggle

Carry #7125 from 4bc4630. The write-lock regression must assert the canonical agents table after V2-to-V1 conversion, including absence of a legacy alias. Correct the API field name in the capacity contract.

Co-authored-by: Epinephrine <27862058+luvs01@users.noreply.github.com>

* docs(codex): name the thread-limit API field consistently

* fix(codex): identify both agents limit keys in conflict warnings

---------

Co-authored-by: Epinephrine <luvs01@hanmail.net>
Co-authored-by: GOAT <goat@localhost>
Co-authored-by: Epinephrine <27862058+luvs01@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants