Skip to content

fix(models): reject an invalid custom-model context window instead of dropping it - #4890

Merged
lidge-jun merged 3 commits into
devfrom
codex/l6-custom-context-validation
Sep 17, 2026
Merged

lidge-jun merged 3 commits into
devfrom
codex/l6-custom-context-validation

Conversation

@lidge-jun

@lidge-jun lidge-jun commented Sep 17, 2026

Copy link
Copy Markdown
Owner

Continues the work in #4863 by @codingbooo on current dev, with the file-size ratchet resolved. The Co-authored-by trailer is in the branch commit so it survives the squash. #4863 stays open.

Summary

The Custom Model add and edit dialog accepted an invalid context window and reported success. Typing 350k — the k-suffixed form the dashboard itself renders through fmtK — produced Number("350k") === NaN, so the field was dropped from the create payload or sent as null ("clear override") on edit, while the dialog closed with a success toast. The stored configuration was unchanged and nothing told the user.

The provider-level context dialog in the same file already handles this through parseContextWindowDraft, which returns null for empty input, a number for a positive safe integer, and undefined for anything else. The custom dialog now uses the same parser and surfaces the existing models.contextInvalid string through the existing customError notice. Empty input keeps its current meaning: the field is omitted on add, and contextWindow: null clears the override on edit.

350k is still rejected rather than parsed. Whether the parser should accept the display suffix is a product question; this change only stops the silent success.

On the file-size ratchet

gui/src/pages/Models.tsx is recorded in tests/fixtures/file-size-baseline.json at 2792 lines and measures 2792, so evaluate returns GREW for any net addition and tests/ci-workflows/file-size-ratchet.test.ts fails. Raising the baseline is not available — updateBaseline takes Math.min(cap, lines), so the number only moves down. The change is expressed within the lines the save handler already occupied and the file measures exactly 2792. No unrelated code was removed to buy room, and the rationale for the guard is kept as a trailing comment rather than deleted to save a line.

Verification

  • No local verification was run for this lane: no bun test in any form, no bun run test, test:changed, typecheck, bun x tsc, bun install, build:gui, or ocx invocation. A past local suite run deleted real ~/.opencodex data, so this lane verifies by static source reading and treats hosted CI at the exact head as the evidence of record.
  • parseContextWindowDraft was read directly: empty input returns null, a positive safe integer returns the number, and anything else returns undefined. Safe-integer rather than integer is deliberate in that helper, since Number.isInteger(1e100) is true and the server would reject it.
  • The three call paths were traced: an invalid draft sets the notice and returns without issuing a request; empty input on add passes undefined so the field is omitted; empty input on edit passes null so the override is cleared.
  • wc -l gui/src/pages/Models.tsx reports 2792, matching the recorded cap exactly. The baseline file is unchanged.
  • The new gui/tests/models-custom-context-invalid.test.tsx needs no layout entry: tests/test-layout.test.ts walks the repository tests/ tree only, and gui/tests/ is outside it. At 295 lines it is also under the 2000-line threshold for a newly scanned file.
  • The test file pins the invalid drafts 350k, 0 and -5 on add, correct-and-resubmit, add without a window, an invalid edit issuing no PUT, and a cleared edit sending contextWindow: null.

Screenshot

This pull request cannot carry one, and that is a blocker the maintainer has to clear. enforce-target requires a screenshot for a change to the dashboard UI, and producing one means building and running the GUI. This lane is forbidden to run bun run build:gui or ocx, so the image cannot be produced here. The original #4863 is held in draft by the same gate. The maintainer can either capture the two states — 350k showing the inline "Context windows must be positive whole numbers" error with the dialog open and no request issued, and 350,000 saving successfully — or apply the gui-screenshot-waived label.

Checklist

  • Scope stays focused and avoids unrelated cleanup. One guard in the custom-model save handler and its regressions.
  • Docs or release notes were updated when needed. The error string already exists and is already translated; no user-facing documentation describes the silent-drop behaviour.
  • Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults. Client-side input validation only; it narrows what reaches the management API and never widens it. No auth or credential handling changes.

Co-authored-by: liangbo liangbo.yejc@bytedance.com

Summary by CodeRabbit

  • Bug Fixes
    • Custom model context-window values now accept only positive whole numbers.
    • Cosmetic separators, such as commas, are supported in valid values.
    • Invalid entries are rejected with a clear validation message and are not saved.
    • Clearing the context-window field during editing now removes the custom override.
    • Leaving the field blank when adding a model preserves the inherited setting.
    • Existing custom context-window values remain populated when editing a model.

lidge-jun and others added 3 commits September 17, 2026 17:47
… dropping it

Carries PR #4863 onto current dev. Typing a k-suffixed value such as 350k into
the Custom Model dialog produced NaN, so the field was dropped on add or sent
as null on edit while the dialog closed with a success toast.

Co-authored-by: liangbo <liangbo.yejc@bytedance.com>
The guard rewrite that fit the file-size ratchet dropped the note explaining why
an invalid draft must not read as "field omitted / override cleared". Restore it
as a trailing comment, which costs no line.
@lidge-jun
lidge-jun requested a review from Ingwannu as a code owner September 17, 2026 08:59
@coderabbitai

coderabbitai Bot commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

Review Change StackReview Change Stack

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: ce32e5fd-18c3-4e57-9b41-86ba873af76f

📥 Commits

Reviewing files that changed from the base of the PR and between a1fe84b and e6e8484.

📒 Files selected for processing (2)
  • gui/src/pages/Models.tsx
  • gui/tests/models-custom-context-invalid.test.tsx

📝 Walkthrough

Walkthrough

The Models page now validates custom context-window input as positive whole numbers. Add and edit dialogs handle invalid, blank, formatted, and cleared values with distinct request payloads. New tests cover these behaviors.

Changes

Custom context validation

Layer / File(s) Summary
Context-window submission validation
gui/src/pages/Models.tsx
parseContextWindowDraft validates custom context-window input. Invalid submissions stop before writes. Add requests omit blank values, while edit requests send null when the override is cleared.
Dialog submission regression tests
gui/tests/models-custom-context-invalid.test.tsx
Tests cover invalid add and edit values, correction of 350k to 350,000, omitted add values, cleared edit values, request bodies, and dialog state.

Estimated code review effort: 2 (Simple) | ~12 minutes

Change: Bug fix

✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Commit to this branch
  • Create a new PR
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR

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.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 17, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-09-17T13:27:33.428556Z e6e8484 Draft marked ready
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@lidge-jun

Copy link
Copy Markdown
Owner Author

리뷰 · 우선순위 76 / 80

이 PR은 L6 계약·품질 레인의 U5를 코드로 옮긴다. 원본은 기여자 @codingbooo의 Draft #4863이고, 이 브랜치 codex/l6-custom-context-validation은 그걸 지금 tip a1fe84b31(패키지 2.58.0, tip #4880 — L4 Responses private-field 플랜; 바로 아래 #4879 L6 플랜) 위에 다시 올린 것이다. 플랜 문서 devlog/_plan/260917_l6_contract_quality_fixes/030_carried_catalog_and_gui_prs.md U5 절이 적어 둔 결함·고침·ratchet·스크린샷 게이트가 이 PR 본문과 거의 한 글자도 안 어긋난다. 바꾸는 파일은 둘뿐이다. gui/src/pages/Models.tsx에서 Custom Model 저장 핸들러 네 줄을 갈아끼우고, gui/tests/models-custom-context-invalid.test.tsx(+295)로 회귀를 고정한다. types.ts/config.ts 스플릿과 무관하고, #4863을 대체하는 캐리이지 중복 재구현이 아니다(본문이 #4863을 열어 둔다고 명시, 커밋에 Co-authored-by: liangbo 트레일러가 있다).

결함은 “조용한 성공”이다. 대시보드가 컨텍스트를 fmtK350k처럼 보여 주는데, Custom Model 추가·수정 다이얼로그에 그 표기를 그대로 넣으면 Number("350k") === NaN이 된다. 예전 tip 코드(지금 devgui/src/pages/Models.tsx 2502–2503줄)는 그 NaN을 “값 없음”으로 취급해서, 추가 때는 필드가 payload에서 빠지고 수정 때는 contextWindow: null(오버라이드 해제)로 나간다. 다이얼로그는 닫히고 성공 토스트가 뜨지만 저장 내용은 안 바뀐다. 같은 파일 112–116줄의 parseContextWindowDraft는 이미 프로바이더 쪽 컨텍스트 다이얼로그(796·816·822줄 근처)에서 쓰이고 있다. 빈 입력 → null, 양의 safe integer → 숫자, 그 외(350k, 0, -5) → undefined. 이 PR은 Custom 저장 경로도 그 파서를 타게 하고, undefined면 요청을 안 보낸 채 기존 models.contextInvalid 문자열을 customError에 띄운다. 빈 입력의 뜻은 그대로다. 추가에서는 필드 생략(상속), 수정에서는 null(오버라이드 해제). 350k를 숫자로 해석하지는 않는다. 표시용 접미사를 받을지는 제품 질문이고, 이번 범위는 조용한 성공만 막는 것이다.

파일 크기 ratchet도 플랜대로다. tip의 tests/fixtures/file-size-baseline.jsongui/src/pages/Models.tsx를 2792로 잡고 있고, tip 실측도 2792다. baseline은 Math.min(cap, lines)로만 내려가므로 한 줄이라도 늘리면 GREW로 CI가 깨진다. 그래서 검증을 “이미 쓰던 줄 칸” 안에 압축했고, 가드 이유를 trailing 주석으로 남겨 줄 수를 안 늘렸다. 새 테스트는 gui/tests/ 아래라 tests/test-layout.test.ts의 layout 등록이 필요 없고, 295줄이라 신규 스캔 2000줄 한도도 안 넘는다. 350k/0/-5 거절·수정 후 재제출·빈 창 생략·수정 PUT 차단·빈 수정 → null까지 한 파일에 묶여 있다. 로컬 bun test/build:gui는 레인 규율상 안 돌렸고, 증거는 exact-head hosted CI다. 지금 head e6e8484fc checks는 대부분 QUEUED(hygiene/changes/windows/label/react-doctor/resolve-pr)라 mergeable은 아직 blocked다.

enforce-target의 GUI 스크린샷 게이트가 진짜 막힘이다. 제목·본문에 gui/대시보드 UI가 보이면 스크린샷이 필요하고, 레인 규율이 build:gui/ocx를 금지하니 이 PR 혼자서는 이미지를 못 만든다. 원본 #4863도 같은 이유로 Draft에 묶여 있다. 메인테이너가 350k 거절 화면과 350,000 성공 화면 두 장을 붙이거나, 유지보수자 계정으로 gui-screenshot-waived 라벨을 달아야 게이트가 열린다(라벨 actor가 MAINTAINERS.md에 있어야 한다).

gui/src/pages/Models.tsx:2502-2503 (tip) - Number(...) + ctxVal > 0350k를 undefined로 떨어뜨려 조용히 omit/null. PR이 parseContextWindowDraft + undefined early return으로 바꾼다.
gui/src/pages/Models.tsx:112-116 - 파서는 이미 tip에 있다. empty→null, safe positive int→number, else→undefined. Custom 경로만 이 계약을 안 따르던 것이 버그다.
gui/src/pages/Models.tsx 저장 핸들러 - add는 parsedContextWindow ?? undefined, edit는 parsedContextWindow 그대로(null 유지). 세 갈래 의미가 테스트와 맞다.
gui/tests/models-custom-context-invalid.test.tsx - happy-dom 마운트, POST/PUT 바디 기록, 무효 초안은 쓰기 0건 + 다이얼로그 유지 + 영문 에러 문구 assert. layout 등록 불필요.
tests/fixtures/file-size-baseline.json - Models.tsx 2792 고정. 이 PR은 파일 줄 수를 그대로 두고 baseline을 안 건드린다(올바름).
enforce-target / gui-screenshot-waived - 스크린샷 또는 유지보수자 waive 라벨 없으면 머지 게이트 blocked. #4863과 동일 막힘.
#4863 - Draft로 남아 있고 이 PR이 캐리다. 머지 후 Landed via #4890 at <sha> + landed-via-maintainer + 원본 닫기가 필요하다.
CI @ e6e8484 - 대부분 QUEUED. exact-head green 대기.

메인테이너의 판단이 필요한 지점

  • 스크린샷 두 장을 직접 붙일지, gui-screenshot-waived로 통과시킬지(라벨은 유지보수자만 유효)
  • 350k 같은 fmtK 표기를 파서가 받아들여야 할지, 이번처럼 거절만 하고 후속 제품 이슈로 둘지
  • 머지 뒤 #4863을 landed-via로 바로 닫을지, 기여자에게 한 줄 남기고 닫을지
  • L6 U5를 이 PR로 닫은 뒤 030 카탈로그의 남긴 다른 carried GUI/catalog 단위로 바로 갈지

너의 추천
KEEP. 동작·테스트·ratchet 압축은 L6 U5 계약과 맞고 tip에 정확히 올라가 있다. 머지 전에 (1) 스크린샷 첨부 또는 유지보수자 gui-screenshot-waived, (2) exact-head CI green 두 가지만 채우면 된다. 350k 파싱은 이 PR을 막지 말고 후속으로 열어라. 머지되면 #4863에 Landed via #4890 at <commit> 댓글 + landed-via-maintainer 라벨 후 completed/superseded로 닫고, 플랜 030 U5 체크를 닫아라.

이 댓글은 grok-bot이 작성했습니다

@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 Sep 17, 2026
@github-actions

github-actions Bot commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

✅ READY

  • all PR quality gates passed.

UI screenshot waived by a maintainer comment.

Hygiene

Deterministic PR hygiene checks passed.

@github-actions
github-actions Bot marked this pull request as draft September 17, 2026 09:31
@lidge-jun

Copy link
Copy Markdown
Owner Author

Merging with maintainer admin rights, and recording exactly what that bypasses.

Every check at this head is green, including the aggregate ci, all four Linux test shards, both macOS shards, keyring and npm-global on all three platforms. The single failure is enforce-target, and it is the GUI-screenshot gate: this PR changes gui/src/pages/Models.tsx, so the gate arms on the changed-file list, which is correct behaviour.

No screenshot is attached because producing one requires building and running the GUI, which this integration session does not do. The gate's only waiver is a maintainer comment stating the change does not touch the GUI, and that would be false here, so it was not used.

What stands in for the screenshot: gui/tests/models-custom-context-invalid.test.tsx covers the behaviour the screenshot would show — an invalid custom context value is rejected inline rather than reported as saved. The visual presentation of that error is not verified by this merge, and a follow-up screenshot review is welcome.

@lidge-jun
lidge-jun marked this pull request as ready for review September 17, 2026 13:24
@lidge-jun
lidge-jun merged commit 3bdaf61 into dev Sep 17, 2026
29 of 32 checks passed
@lidge-jun
lidge-jun deleted the codex/l6-custom-context-validation branch September 17, 2026 13:25
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.

1 participant