fix: resolve combos by what they are, and check the documented discovery limits (#5216, #5215) - #5294
Conversation
…t runs Two user-visible strings on the compaction-routing surface described behaviour the code does not have. The panel decided whether a selection was a combo by testing for a "combo/" prefix. A combo reached through an alias carries no prefix, so it was described as an ordinary provider and none of its targets were named -- the answer existed and the operator could not see it. The panel now asks what the selection resolves to: the combo list is keyed by the public model id the server already computes, which is the alias when one is set and "combo/<id>" otherwise, so both spellings answer the same way. It reads that list through parseComboList, the same reader the combo workspace uses, so the selector rule is not spelled out a second time here. The combo warning told the operator that a covered compaction goes to every target, including failover targets. It does not. core-combo.ts dispatches one target per loop iteration, returns as soon as one responds, and advances only after a retryable failure. An operator reading the old text would budget fan-out cost and fan-out latency for something that never happens. The warning now says the targets are attempted in order and the first that answers is used, which is both what happens and what someone debugging a slow compaction needs. Wording changed in all ten locales. The regression covers the aliased combo the prefix test could not see, and asserts the ordering sentence rather than the fan-out claim. Closes #5216
The provider guides restate a byte ceiling and a row ceiling for thirteen fixed-host presets, in eight pages, and nothing compared any copy to the registry. #5198 fixed a preset count that had drifted across sixteen files for months for exactly that reason; these limits are the same shape one layer down. Every number is now read from that preset's modelDiscovery and asserted against every shipped guide, so lowering a ceiling fails in all eight locales at once instead of leaving seven translations describing the old one. A grouped section must first agree in the registry before one sentence may speak for two presets, which is what makes the Nscale/Vultr and Command Code sentences legitimate rather than convenient. Sections are located by brand name and the presence of a KiB or MiB token, not by a translated sentence. A restated anchor phrase is the same hand-copied value the guard exists to remove, and the brand names are Latin in all eight published locales. The byte ceiling is compared as an exact token set rather than a substring, so a stale number left beside the current one fails. The structure record claimed the guides carried identical limits. That claim was false when it was written: the Korean guide had no Featherless section, so it documented twelve of the thirteen limited presets. The section is added and the prose is replaced by a description of what is actually asserted. Closes #5215
…lane Combo target lookup read a plain object by the selected model id. A combo id is free-form, so an alias of "constructor" or "toString" resolved to an inherited Object member and the renderer tried to join a function. Read it with Object.hasOwn. Recognizing a combo only through the fetched list lost the canonical prefix as a signal of its own. When /api/combos has not answered yet or failed, a "combo/x" selection was described as an ordinary provider named "combo" -- worse than the alias gap this lane set out to fix, because that path is reachable whenever the management API is briefly unavailable. The prefix is kept as an independent signal and the target names fall back to the existing "its configured target providers" wording. The documentation guard compared the row ceiling as a substring of the whole paragraph, so the byte ceiling's own digits could satisfy it: a Hyperbolic paragraph saying "256 KiB and 128 raw rows" would have passed an expected 256 rows. Row numbers are now read from the prose with the unit tokens removed. All 104 locale/section combinations still pass, verified by transcribing the test's own logic over the eight guides.
Why the recovery path cannot live in the dashboard, what each of the six targets needed, the differential between the two workspace pull requests with the three findings that decide their sequencing, and the one src/ defect this lane identified and left stated rather than half-fixed.
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. |
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Repository: lidge-jun/opencodex/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (18)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. |
…ect key A combo's public model id is free-form and operator-configured, and readComboProviders wrote it straight into an object literal. That is a prototype-pollution sink on the write side, and the read side returned an inherited member for an alias of "constructor" or "toString" -- the previous commit guarded the read with Object.hasOwn and left the write as it was. A Map removes both. There is no prototype to shadow, the guard disappears, and the failed-fetch fallback returns an empty Map rather than an empty object, so the two branches keep the same type.
![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: 5445e38a24
ℹ️ 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".
| ? targets.map(target => (target as { provider?: unknown })?.provider).filter((value): value is string => typeof value === "string") | ||
| : []; | ||
| result[(combo as { id: string }).id] = [...new Set(providers)]; | ||
| for (const combo of parseComboList(payload)) { |
There was a problem hiding this comment.
When compactionRouting.model is the canonical combo/<id> for a combo that also has a public alias, /api/combos returns only the alias in combo.model, so this map has no entry for the saved canonical selector. That selector remains valid by design (resolveComboId explicitly accepts canonical IDs for backward compatibility), and combo edits do not migrate compactionRouting; the prefix fallback therefore recognizes the combo but displays “its configured target providers” instead of the actual providers, regressing the disclosure that previously worked for canonical IDs. Index each row by both combo.model and comboModelId(combo.id).
AGENTS.md reference: gui/AGENTS.md:L9-L10
Useful? React with 👍 / 👎.
| "compactionRouting.autoNotice": "Automatic compaction runs on its own, so a long conversation can be sent to that provider without you asking for it.", | ||
| "compactionRouting.providerWarning": "With this setting, every covered compaction request sends the full conversation contents to {provider} for summarization.", | ||
| "compactionRouting.comboWarning": "With this setting, every covered compaction request sends the full conversation contents to every target of combo {combo} ({providers}), including failover targets, for summarization.", | ||
| "compactionRouting.comboWarning": "With this setting, every covered compaction request sends the full conversation contents to combo {combo} for summarization. The combo attempts its targets ({providers}) in order and uses the first that answers, so any one of them can receive the conversation.", |
There was a problem hiding this comment.
For combos using round-robin, random, least-used, or reset-window, this warning is inaccurate: pickComboTarget in src/combos/resolve.ts chooses the first target using weight, state, usage, or quota-reset data rather than the displayed configured order. Because the same copy is rendered for every strategy, operators can be told that (provider-a, provider-b) is attempted in that order when the request may start with provider-b; use strategy-neutral wording or include the combo strategy in the warning.
AGENTS.md reference: gui/AGENTS.md:L9-L10
Useful? React with 👍 / 👎.
| expect(rowNumbers(paragraph), `${guide.path} ${section.brand} row ceiling`) | ||
| .toContain(rows); |
There was a problem hiding this comment.
For Featherless, every guide paragraph contains two standalone 100 values: the request asks for the top 100 models and discovery is capped at 100 raw rows. If the raw-row limit sentence is removed or becomes stale while the unrelated query-size value remains, rowNumbers(paragraph).toContain(rows) still passes, so the new guard does not actually ensure that this documented row ceiling exists. Match a number in row-limit context or require an unambiguous occurrence rather than accepting any integer in the paragraph.
Useful? React with 👍 / 👎.
⏳ DRAFT
What to do
Automatic draft conversion failed (token cannot change draft status). Please convert this pull request to a draft manually. The required |
리뷰 · 우선순위 41 / 80이 PR은 압축 라우팅 화면이 콤보를 잘못 읽던 부분과, 공급자 안내 문서에 손수 적힌 discovery 한도가 레지스트리와 어긋날 수 있던 부분을 고칩니다. 베이스는 예전에 화면은 이름이 문서 쪽은 13개 고정 호스트 프리셋의 바이트·행 한도를 레지스트리 라인 - 라인 - 라인 - 콤보 경고 문구(열 개 로케일) — “순서대로 시도”는 failover에 가깝습니다. round-robin·random·least-used도 한 대상만 쓰지만, 고르는 순서는 “목록 순서”가 아닐 수 있습니다. 팬아웃 오해는 풀렸고, 전략 이름까지 맞추지는 않았습니다. 라인 - CI — 메인테이너의 판단이 필요한 지점 레인 G 분석(#4560 vs #5068 순서, 이메일 마스킹, 너의 추천 #5216·#5215 범위는 방향이 맞고, 적대 패스로 잡은 세 구멍(상속 멤버, 접두사 폴백, 행 한도 숫자 혼동)도 코드에 반영되어 있습니다. 이 댓글은 grok-bot이 작성했습니다 |
React Doctor's js-flatmap-filter fired on the map().filter(Boolean) this lane introduced at CompactionRoutingPanel.tsx:51 -- one new warning in one file, and the job's blocking threshold is warning. flatMap does the same work in a single pass. The related js-combine-iterations rule is switched off in gui/doctor.config.json, but this is a different rule and is enabled, so this is a real new finding rather than an accepted one.
추가 리뷰 · 우선순위 32 / 80이전 리뷰(HEAD 라인 - 라인 - 라인 - 드래프트 — 아직 draft입니다. 머지 전에 Ready로 올려야 합니다. 라인 - 라인 - 라인 - 콤보 경고 문구 — “순서대로 시도”는 failover에 가깝고, round-robin 등 다른 전략 이름까지 맞추지는 않았습니다. 팬아웃 오해는 이전 커밋에서 이미 풀렸습니다. 메인테이너의 판단이 필요한 지점
너의 추천 이전 차단이었던 이 댓글은 grok-bot이 작성했습니다 |
1 participant
Summary
Roadmap item 16 asks for connect → confirm → change → check state → recover as one flow. This PR
delivers the two targets that were fully specified and self-contained, and records the analysis
the rest needs before it can be sequenced. It adds no new control plane and no new widget.
The finding that shapes the flow
The dashboard is served by the proxy:
startServerbinds the listener and that same listenerserves
gui/dist. When the proxy stops there is no surface left to render a recovery panel, sothe state #5261 was reported in — injected routing pointing Codex's own built-in provider at a
dead loopback port — is one the dashboard cannot be part of getting out of. Recovery belongs to
the surfaces that survive the proxy, which is where #5267 already put it. What the dashboard owes
is disclosure before the fact: it is the only surface present when the integration is applied
and the one that will be gone if the proxy later stops.
#5216 — the compaction panel described behaviour the code does not have
The panel decided whether a selection was a combo by testing for a
combo/prefix. A comboreached through an alias carries no prefix, so it was described as an ordinary provider and none
of its targets were named. It now asks what the selection resolves to, keyed by the public model
id the server already computes, read through
parseComboList— the same reader the comboworkspace uses, so the selector rule is not written down twice. The canonical prefix is kept as
an independent signal, because it is the only one left while
/api/combosis unanswered orunavailable, and losing it there would call a combo "the provider named combo".
The warning claimed a covered compaction goes to every target including failover targets.
core-combo.tsdispatches one target per loop iteration, returns as soon as one responds, andadvances only after a retryable failure. An operator reading the old text would budget fan-out
cost and latency for something that never happens. Reworded in all ten locales.
#5215 — hand-copied registry values had no check behind them
Thirteen presets restate a byte ceiling and a row ceiling across eight guides. #5198 fixed a
preset count that had drifted across sixteen files for months for exactly this reason; these are
the same shape one layer down. Every value is now read from that preset's
modelDiscovery, solowering a ceiling fails in all eight locales at once.
Sections are located by brand name plus the presence of a
KiB/MiBtoken rather than by atranslated sentence, because a restated anchor phrase is the same hand-copied value the guard
exists to remove. The byte ceiling is compared as an exact token set and the row ceiling is read
with the unit tokens removed, so neither can pass on the other's digits.
The guard earned its keep immediately.
structure/ops/docs-and-release.mdasserted in prose thatthe guides carry the same limits, across sixteen-plus files, verified by nobody. That claim was
false when it was written: the Korean guide had no Featherless section, so it documented twelve
of the thirteen limited presets. The section is added and the prose is replaced by a description
of what is actually asserted.
#4560 and #5068 — analysed, deliberately not merged
The instruction was to review the real difference and consolidate only duplicated screens. They
are not the same feature. 46 and 39 files, intersecting in 31, of which only nine are
byte-identical. #4560 is the UI foundation (responsive grid, collapsible rails, Cockpit import,
the quota-analysis suite); #5068 is the pool follow-up (pool strategy persistence and preview,
per-account refresh, token estimates, plan badges, focus trapping).
Three findings decide their sequencing, and none of them is "they overlap":
ProviderAccountCard.tsxdrops the Grok couponbadge and the
ProviderAccountQuotafallback, so landing it after feat(gui): redesign provider accounts workspace with adaptive grid, collapsible rails, and quota controls #4560 would silentlyrevert them.
account-quota-analysis.tsaddsrawEmailandprefers an unmasked value where feat(gui): redesign provider accounts workspace with adaptive grid, collapsible rails, and quota controls #4560 uses the management API's projected email. That is a
privacy boundary and needs explicit review against
emailMaskingEnabledbefore either lands.gui/src/styles.css, whose committed cap is 2,958 lines; its head is 3,186. The ratchet onlymoves downward, so the remedy is the sibling file feat(gui): redesign provider accounts workspace with adaptive grid, collapsible rails, and quota controls #4560 already uses, not a new number.
Both are 28 commits behind
devand conflict on all ten locale catalogs through #5197.Remaining scope (these issues stay open)
engine, and phase 3 is its dashboard surface; carrying an unlanded engine and building its
surface in one branch would put the authorization boundary feat(codex): plan and apply a Codex CLI update from bound evidence (#2811) #5016 is built around under review
twice.
codexStatusinsrc/server/management/native-integration-routes.tsderivesstatefromconfig.clientIntegrations?.codexalone. It reports desired configuration, not what iscurrently applied — it never reads
config.toml, the routing kind or the catalog pointer,though
src/codex/injected-marker.tsalready exposes the predicates. So the dashboard cannotanswer "what is applied right now", and names no undo path at apply time. The bounded fix is
recorded in the lane devlog; it changes a DTO every native client shares plus ten locale
catalogs, so it is its own commit set rather than an addendum to this one.
Verification
That review found all three defects fixed in
af33bc39d7: a combo alias ofconstructorortoStringread an inherited Object member and was joined as a target list; recognizing acombo only through the fetched list lost the prefix signal when
/api/comboswas unavailable;and the row-ceiling check could pass on the byte ceiling's digits.
before committing, rather than assumed. That is how the Korean gap surfaced and how the byte
and row values were confirmed to already agree with the registry everywhere else. 104 of 104
locale/section combinations resolve to exactly one paragraph with one unit token.
(
gui/src/styles.cssis untouched here, which is precisely why feat(gui): isolate pool-workspace follow-up on fresh dev #5068 cannot land as itstands); the new test is registered in both
scripts/test-layout/layout.jsonandtests/fixtures/test-layout-expected.json; all ten locale catalogs keep an identical key setbecause only values changed; and no count is restated — the limits are derived.
dev53725b0decand pushed at that base, so hosted CI runs at the exact head.bun run test, individualbun testfiles,bun run typecheck,bun run lint:gui,bun run build:gui,bun run structure:check,bun install, and liveocxexecution. This lane verifies by static review plus exact-head hosted CI only; the CI runon this head is the test evidence.
Closes #5216
Closes #5215
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
UI change evidence (no screenshot — GUI build is out of scope for this lane)
enforce-targetarms the screenshot gate fromguiPathsChanged(changedFilePaths), and this PRchanges
gui/, so the gate applies. I cannot satisfy it: producing a screenshot needsbun run build:guiand a running proxy to serve the dashboard, and this lane forbids builds andlive
ocxexecution. Below is the evidence a screenshot would have carried, so a maintainer canjudge whether to waive it with the
gui-screenshot-waivedlabel or a comment.Layout is unchanged. No element is added, removed, resized or moved. The panel keeps its
section.panel, the same threeSelectcontrols and save button, and the same singlediv.notice-warn[role="note"]in the same position beneath them. The only change is which oftwo existing strings renders inside that note, and the wording of one of them.
gateway/cheapcombo/compactquickpick/api/combosis unavailableThe third row is the user-visible defect #5216 reports. The second row is the wording correction.
Rendered-output coverage stands in for the image:
gui/tests/compaction-routing-panel.test.tsxmounts the real component against a fixture
/api/combospayload and asserts the exact text of[role="note"]in each of those states, including a new aliased-combo case, a newcombos-unavailable case, and a new case for an alias that shadows an
Objectmember. Thelocale catalogs keep an identical key set, so no layout reflow from a missing key is possible.