Skip to content

fix(codex): validate subagent limit boundaries before writes - #7276

Merged
lidge-jun merged 3 commits into
lidge-jun:devfrom
luvs01:codex/subagent-limit-boundaries-followup-20261017
Oct 24, 2026
Merged

lidge-jun merged 3 commits into
lidge-jun:devfrom
luvs01:codex/subagent-limit-boundaries-followup-20261017

Conversation

@luvs01

@luvs01 luvs01 commented Oct 17, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

  • Display zero children when global V2 stores a total thread limit of one, in both the Models selector and folded summary. Entering one child then sends a total of two instead of being treated as an unchanged value. V1 and hybrid mode retain child-count units.
  • Validate safe integers at both request entry points. The GUI rejects unsafe child counts and unsafe values after adding the V2 root slot before sending a PUT. The API returns 400 for unsafe limits before feature toggles, convergence or configuration writes.
  • Document the display and validation boundaries. This follows the existing upstream discussions about root-only display and unsafe limit validation.

Verification

Current head: 5ea405247bc3c674cf1103547f4895edcceabb62, a normal merge of 21db48ca329b97f18f3e3e667c870c76bec11d14 and dev@c4d4bb2da2162e23c314befafc5ec104292902a0. All seven PR-specific code, test and documentation blobs remain unchanged; no conflicts or manual source edits were needed.

  • Official Bun 1.4.4+38afee6c8, Windows: backend/API limit tests 19 passed, 120 assertions; GUI limit tests 20 passed, 71 assertions. These use isolated homes and fixture DOM/fetch, not live capacity or installed-app verification.
  • Root and GUI TypeScript, GUI production build, structure SSOT, privacy, file-size ratchet and PR-relative diff checks passed. Existing GUI duplicate-key/large-chunk warnings remain. Sparse-excluded tracked inputs were materialized unchanged; no dependencies were installed.
  • Current-head CI passed. The previous 21db48ca CI and Bun 1.4.3 checks remain historical evidence, not results for this new head.
  • Current documentation build is unverified because Astro dependencies are absent in the local checkout. The prior successful documentation build and unchanged document blobs are retained as historical evidence.
  • Full local root/GUI suites, native effective capacity, production configuration and host reload were not exercised. Owner GUI/API-storage review and explicit security review remain required before merge.
  • The existing CodeRabbit review covers the original patch. Its 16.67% docstring advisory against an 80% target was not waived or reported as passed; no actionable review thread remains.

Historical screenshot fixture

The GUI blobs are unchanged, so this synthetic-fixture capture is retained. It is not a fresh native-app validation: root-only V2 displays 0 children; applying 1 child sends {maxConcurrentThreadsPerSession:2}. The PNG is pinned to its asset commit and its previously verified SHA256 is 6d7a37c5584ada1f376c4259f4c6b802d2122b2e7fec6e588b4db2cbed47d18f.

Root-only V2 selector and one-child edit with synthetic API fixtures

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.
  • Current head and validation scope are recorded.
  • The existing screenshot is attached through an immutable asset URL.
  • Maintainer/code-owner review is complete.
  • My PR is ready for review.

Summary by CodeRabbit

  • Bug Fixes
    • V2 sub-agent limits now display as the number of child agents, excluding the root agent. Saving a child limit sends the corresponding total thread limit.
    • Invalid or unsafe limits are rejected before settings are changed. The largest safely supported limit remains accepted, including when V2 is disabled or hybrid mode is used.
  • Documentation
    • Clarified how child limits relate to V2 thread limits, including root-only settings and hybrid mode.

Display the actual zero-child count for root-only V2 settings and preserve mode-specific units when editing. Reject unsafe child and converted backend limits before PUT or configuration mutations, with focused regressions and contract documentation.
@coderabbitai

coderabbitai Bot commented Oct 17, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: lidge-jun/opencodex/.coderabbit.yaml
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: 6afe1131-d186-4bfe-b954-a220c3573e0f
📥 Commits

Reviewing files that changed from the base of the PR and between 2a10a2b and 211a5ea.

📒 Files selected for processing: 7
  • docs-site/src/content/docs/reference/configuration/agents.md
  • gui/src/pages/Models.tsx
  • gui/src/pages/models-shared.ts
  • gui/tests/models-subagent-limit.test.tsx
  • src/server/management/agent-settings-routes.ts
  • structure/subagent-capacity.md
  • tests/codex-integration/codex-agent-thread-limit.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.

📝 Walkthrough

The Models page now displays V2 limits as child counts and converts edits to API thread-limit units. The UI and management API reject values outside safe-integer bounds. Tests and documentation cover root-slot conversion, hybrid-mode behavior, and API validation.

Changes: Thread-limit units and validation

Layer / File(s) Summary
UI child-count conversion and validation
gui/src/pages/models-shared.ts, gui/src/pages/Models.tsx, gui/tests/models-subagent-limit.test.tsx, docs-site/src/content/docs/reference/configuration/agents.md
V2 totals that include a root slot display as child counts, with a stored limit of 1 displaying as 0. The UI converts child counts to API units when required and validates both the child count and converted limit as safe integers. Tests cover root-only display, custom saves, hybrid-mode behavior, and safe-integer boundaries.
Management API safe-integer validation
src/server/management/agent-settings-routes.ts, tests/codex-integration/codex-agent-thread-limit.test.ts, structure/subagent-capacity.md
The /api/v2 endpoint now requires maxConcurrentThreadsPerSession to be a safe integer of at least 1. Integration tests cover rejection before side effects and acceptance of Number.MAX_SAFE_INTEGER. The documentation describes the API limit behavior.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 211a5

The documented limit conversion and validation changes have no identified merge-blocking issue; normal checks remain appropriate.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 5 files. (2 skipped: 2… Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Title check Passed The title clearly and concisely summarizes the main change: validating sub-agent limit boundaries before feature toggles, API updates, and configuration writes.
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Full details: Docstring Coverage

Explanation

Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 5 files. (2 skipped: 2 unsupported.)

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • 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 17, 2026
@luvs01
luvs01 marked this pull request as ready for review October 17, 2026 15:08
@Ingwannu

Copy link
Copy Markdown
Owner

Scoped review at 21db48ca32: the child-count conversion now shows root-only V2 as zero and validates both the child value and converted backend value before a write; PUT uses Number.isSafeInteger before combined mutations. Keep global enabled versus hybrid/catalog-selected V2 units distinct, and keep zero display separate from the minimum new edit of one child. Current Ubuntu CI succeeded. Please retain owner GUI review and the current API/storage documentation contract in readiness; this comment covers the numeric boundary only, not an autonomous UI decision or merge approval. No real config writes were made.

@lidge-jun

Copy link
Copy Markdown
Owner

Independent source review (gpt-6.1-sol, dispatched by the triage manager, 2026-10-24) at head 5ea405247b:

VERDICT: PASS
LANE_ALL_REQUIRED: no

Reviewed head 5ea405247bc3c674cf1103547f4895edcceabb62. No actionable findings.

  • Correctness: root-only V2 displays zero (gui/src/pages/models-shared.ts:207); GUI validates children and converted totals before PUT (gui/src/pages/Models.tsx:1242).
  • Security/API-storage: unsafe limits return 400 before toggles, locks or writes (src/server/management/agent-settings-routes.ts:383). No introduced auth, credential or permission-boundary regression found.
  • Regression coverage meaningfully exercises rendered zero display, one-child edits, hybrid units, safe-integer boundaries and rejection without side effects (gui/tests/models-subagent-limit.test.tsx:213, tests/codex-integration/codex-agent-thread-limit.test.ts:148).
  • Existing test registrations remain valid. layout.json stays at 1,999 lines; changed files respect ratchets; updated structure documentation is 79/600 lines. PR template and screenshot are present.
  • Targets dev, is 4 commits behind, and is mergeable without conflicts. No unresolved review threads. Formal approval remains outstanding.
  • Exact-head PR CI succeeded. macOS/Windows were skipped; no exact-head lane=all run exists. This arithmetic/validation change introduces no platform-specific behavior requiring it.

NOT RUN: tests, typecheck, builds, runtime/UI verification—read-only review as requested.

This is a read-only source review; merging still requires the gates named above (exact-head CI, lane=all where marked, and maintainer acceptance where required).

@lidge-jun

Copy link
Copy Markdown
Owner

Integration evidence (owner decision decision:3499cac6, approved, D11): independent gpt-6.1-sol review PASS — root-only V2 displays zero children, GUI validates children/converted totals before PUT, and the API rejects unsafe limits with 400 before toggles, locks or writes; regressions cover display, hybrid units, safe-integer boundaries and no-side-effect rejection. Hosted Ubuntu CI was green on this head (39963360041). No platform-specific behavior.

Static merge simulation on current dev 6426c42 (this Mac): dev=6426c42b256425a3f78e09826353dad63f68fef6 base=c4d4bb2da2162e23c314befafc5ec104292902a0 pr_files= 7 overlap=[]; merge clean; install exit=0; gui install exit=0; tsc exit=0; tsc doctor exit=0; gui tsc -b exit=0; gui lint exit=0; structure:check exit=0; privacy:scan exit=0; layout+ratchet guards exit=0 52 pass 0 fail ;

Merged by the triage manager as maintainer integration (MAINTAINERS.md, dev only) under the owner's static-judgment rule while GitHub Actions is degraded; dev push CI will be checked when Actions recovers.

@lidge-jun
lidge-jun merged commit 8aa8206 into lidge-jun:dev Oct 24, 2026
37 checks passed
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.

3 participants