Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
# Phase 010 — done

#6058's routed-model capability editor is on `dev`. It merged through #6105 as `3401e1ee73` from exact head `84be8f48a6`, and the contributor's commit keeps its authorship. #6058 was closed with credit and a summary of what changed on top of it. Post-merge `dev` Cross-platform CI was dispatched as run 36344698221 on `3401e1ee73`.

## Changes after `011_model_settings_evidence.md`

Review on #6105 found real defects that the lane's own audit had missed:

- **Codex review.** Restore left an exact legacy `modelInputModalities` entry in force. The catalog ladder fallback also looked up a bare model ID where routed catalog slugs are `provider/model`. Both are fixed, and each fix has a test that fails without it.
- **CodeRabbit.** A 4xx rejection used to push the dialog into the unknown-outcome state; a rejection writes nothing, so the dialog now stays editable with translated copy. `--modalities ""` normalized to an implicit clear and is now rejected. The Codex catalog warning now names each locale's own "Sync now" button. French copy and the API reference were also completed. The request to cache registry enrichment per provider was declined as a trivial-priority refactor with no correctness effect.
- **React Doctor.** It blocks on any warning, and it flagged `[...values].sort()` in contributor code, now `toSorted()`. Running `npx react-doctor@0.9.11 --scope changed --base origin/dev` in `gui/` before pushing catches this locally.

## What did not go well

- A second local `test:changed` run hit the suite's 900-second cap after waiting 18 minutes for another worktree's test lock. It is recorded as interrupted with 0 failures; CI covered that head.
- `enforce-target` runs on a PR head are routinely superseded by `status`-event runs in the same per-PR concurrency group, so the check on the head shows `cancelled` even though the gate raised no objection. The coordinator's adjusted merge rule replaced repeated rebase-and-wait cycles with a union-tree check. The first rebase under it was still required because #6112 touched both test-layout inventories and the structure doc.
- The unit tests did not catch the dialog menus opening behind the modal; only a real browser click did. Future GUI phases need at least one real-browser pointer pass over every control in a dialog, since keyboard coverage alone would have missed it.

## Next

Phase 020 (#4932) starts from `dev` `3401e1ee73` on branch `codex/t4-gui-ux-combo-sidecar`. The explorer map corrects two claims in `020_combo_sidecar.md`: a missing provider is already rejected by `comboConfigError`, and the request-only field was never persisted under `combos`. It also confirms that writing a text-only declaration is the enrollment mechanism, and that phase 010's editor writes the same axis. Phase 030 (#5617) is deferred because its own acceptance gate fails: there is no migration or rollback from `disabledModels`, and the provider UI still overlaps the account-pool carry #6106.
46 changes: 46 additions & 0 deletions devlog/_plan/260927_release_train_4/gui-ux/020_combo_sidecar.md
Original file line number Diff line number Diff line change
Expand Up @@ -16,3 +16,49 @@ In an isolated proxy, create a mixed combo from one image-capable and one text-o
## Rollback and boundary

Do not overwrite sibling per-model context/reasoning axes. If the generated modality declaration cannot be distinguished from an operator's existing declaration, the implementation must preserve the existing one and explain that behavior; rollback must never delete operator-owned facts.

## P revalidation for `wp2` (2026-09-28, `dev` `3401e1ee73`)

Continuity: phase 010's D (`012_model_settings_done.md`) closed with #6105 merged and pointed here, adopting the explorer's corrections. That direction stands, with the corrections below.

**Corrected claims.** A missing configured provider is already rejected by `comboConfigError` before #4932's silent skip could run (`src/server/management/combo-routes.ts:225-231`, `src/combos/types.ts:275-281`). An explicit provider check stays as a guard, but there is no reachable 200 to fix. #4932 never persisted `visionSidecarTargets` under `combos`, because `stored` is built from the normalized combo. Both defects that do hold are kept: #4932 overwrites an existing declaration with `["text"]` (`mergeModelCapabilities`, `src/config/provider-validation.ts:419-432`), and it classifies audio-only rows as sidecar-eligible, although the runtime requires `text` (`src/vision/eligibility.ts:136-146`). It also lacks `vi.ts`.

**Enrollment mechanism, verified.** `isModelVisionSidecarConsumer` (`src/vision/eligibility.ts:136`) treats an exact `modelCapabilities[id].inputModalities` containing `text` but not `image` as a sidecar consumer. The catalog then advertises `image` for that row (`src/codex/catalog/model-hints.ts:285-294`), whether or not the sidecar is enabled. After enrollment, `GET /api/models` reports the member as image-capable, so the existing `comboImagesSupported` check passes on reload. Actually describing an image needs the sidecar enabled with a usable backend (`src/vision/plan.ts:195-217`).

### Decisions

1. **Server (`combo-routes.ts`).** Accept a top-level, request-only `visionSidecarTargets: {provider, model}[]` on combo create/update. Validate every target before any mutation:
- The target is an exact member of the submitted combo.
- The provider exists and is routed (not `openai` native or a combo).
- The combo's `imageInput` is not `disabled`.
- The member's existing declaration, read the way the runtime reads it (exact `modelCapabilities`, then the legacy record), is absent. An existing text-only declaration is a no-op. A declaration that includes `image`, or one that lacks `text`, is a 400; it is never overwritten.

Write each `["text"]` declaration into a copied `modelCapabilities` row, preserving sibling capability fields, in the same `commitProviderPatch` transaction as the combo write, so there is one save. The field never lands under `combos` and is not echoed back.
2. **GUI classification (`combo-capabilities.ts`).** Each member is classed from its `/api/models` row as `image` (modalities include `image`), `sidecar` (known modalities with `text` and without `image`), or `blocked` (no known modalities, or no `text`). The Image input switch is available when no member is `blocked` and at least one member is `image` or `sidecar`. Saving with images on sends every `sidecar` member as a target.
3. **Hint (`combo-workspace-controls.tsx`).** When `sidecar` members exist, the hint names them exactly (`provider/model`) and says the Vision Sidecar will describe images for them. If `GET /api/sidecar-settings` reports the sidecar disabled, a warning line says images for those members need it turned on, linked to its settings. When a member is `blocked`, the hint names it and the reason (modalities unknown, or no text input), and the switch stays off. Save errors surface the server's rejection through the existing error path. The save stays one primary action.
4. **Copy.** New keys go into all ten locales, including `vi`.
5. **Tests.** `tests/routing/combo-management-api.test.ts` gets each rejection with config unchanged and no save: non-member target, disabled image input, image-capable or audio-only declaration, and native provider. It also covers the success write that preserves sibling fields, the text-only no-op, and the absence of the field under `combos`. `tests/gui/combo-workspace-data.test.ts` gets classification and request shape; a `gui/tests/combo-workspace-*.test.tsx` case covers the rendered hint and the save body.

Deferred: a provenance marker for generated declarations. Rollback keeps operator facts, and the declaration can be cleared per model from phase 010's editor.

### Architect reflection (`01a0e45c-65f8-7023-8894-501c6910b602`, `gpt-6-sol`): GAPS, folded

1. **Precedence.** Server validation reuses the runtime predicates instead of re-deriving precedence. The effective declaration is read in the runtime's order: exact `modelCapabilities`, then the operator's custom row, then `noVisionModels`, then `modelRecordValue` over the legacy record (exact, colon-family, case-fold) (`src/vision/eligibility.ts:136-146,200-218`, `src/reasoning-effort.ts:124`). For each target:
- If `modelAcceptsImageInput` says the model already takes images, return 400, because a text-only declaration would hide a real capability.
- If the effective declaration exists and lacks `text`, return 400.
- If the registry-enriched provider already makes it a consumer (`isModelVisionSidecarConsumer`), do nothing, since the catalog already advertises image.
- Otherwise, including a custom row declared `["text"]` (which the catalog's custom-row path does not cover, `src/codex/catalog/routed-gather.ts:730`), write the exact `modelCapabilities[model].inputModalities = ["text"]` and preserve sibling fields.

The reload test asserts `/api/models` reports `image` for each enrolled member.
2. **Sidecar status.** `Combos.tsx` loads `GET /api/sidecar-settings` separately from the three workspace requests, so a failure never blocks the workspace. It reads `vision.enabled` (`src/server/management/config-routes.ts:280,815`). A missing or failed read shows no warning and does not guess. The warning appears only when `vision.enabled === false` and `sidecar` members exist.

The integration point is confirmed: create, update and rename share `PUT /api/combos` (`combo-routes.ts:132`), with one save at `:343`, so `commitProviderPatch` can wrap the mutation and that save.

### Independent A (`01a0e45f-c395-79a0-b1cb-41c43abde891`, `gpt-6-sol`): GO-WITH-FIXES; held for the next train

Before B started, the coordinator narrowed train 4 to PRs that were already open or nearly finished. No implementation started, so #4932 is **held for the next train** with a comment on the PR. The audit findings below are part of the plan the next train starts from:

1. **High.** Enrollment makes a member advertise `image`, so catalog modalities alone reclassify it as native-image after reload, and the "sidecar is off" warning disappears. Classify with the declaration `/api/models` exposes separately (`inputModalitiesDeclared`, `src/server/management/model-rows.ts:388-404`), and test save, reload, then disable the sidecar.
2. **Medium.** A custom row's `/api/models` row is rebuilt from `customModels` (`model-rows.ts:323-334,373-376`), so "every enrolled member reports `image`" cannot hold for a custom row. Either align that projection with sidecar coverage or test the custom-row outcome separately.
3. **Medium.** Show enrollment and sidecar wording only while the combo's image input is enabled. Test an existing `imageInput: "disabled"` combo, then switching it on.
4. **Medium.** Wrapping the save in `commitProviderPatch` needs direct tests: a successful rename still migrates identity and `disabledModels`, and a failed save restores the combo, the declaration and the rewritten references. Both guides must say that removing a member leaves its provider-level declaration until the operator clears it (phase 010's editor can).
17 changes: 17 additions & 0 deletions devlog/_plan/260927_release_train_4/gui-ux/040_dispositions.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,3 +13,20 @@
- **#4189:** current ZCode client integration and Z.AI provider APIs are separate. The report does not identify a ZCode upstream login/API contract. Ask the issue author in English whether they mean the ZCode client using OpenCodex, Z.AI API-key upstream, or a distinct login; leave open pending answer and do not add a fake `zcode` provider card.

For every deferred PR or issue, post one evidence-backed English comment with the disposition. Do not close a contributor PR merely for being large or stale. An adopted contributor PR closes only after a replacement lands and a credit trailer plus replacement link are present.

## Final dispositions (2026-09-28)

The coordinator ended train 4's implementation work early and told the lane to leave hold comments on unstarted candidates. Every comment below is in English with file-level reasons, and every PR and issue stays open.

| Item | Outcome | Reason |
|---|---|---|
| #6058 | Carried, merged in #6105 (`3401e1ee73`), closed with credit | See `010`–`012`. |
| #4932 | Held for the next train | The plan passed audit with fixes, but implementation had not started when the scope narrowed. The plan and audit are in `020_combo_sidecar.md`. |
| #5617 | Held | There is no migration or rollback between `globalDisabledModelIds` and `disabledModels`. It conflicts in `app-routing.ts`, `Providers.tsx` and `structure/config.md`, overlaps #6106, and has four open CodeRabbit findings. |
| #4649 | Held | It needs a credential-handling security review. A remembered token survives logout, the tests target the old `/api/settings` path, the docs would become false, a screenshot is committed, and the branch is 317 commits behind. |
| #5932 | Held | Every file belongs to the account-pool work (#6106), and `provider-workspace/types.ts` conflicts. |
| #2355 | Held | 2,971 commits behind with eight conflicting files, and screenshots are committed under `docs/pr-assets/`. |
| #5408 | Held | 67 files and 7,790 added lines, six conflicts, overlap with #6106, and eleven committed screenshots. It should be split into single-behavior PRs. |
| #4644 | Open | #4649 is held. |
| #3379 | Open | Journal deletion and custom ranges have landed; renaming the selector (picker/account area) remains. |
| #4189 | Open, question asked | ZCode (client) versus Z.AI (`zai` key provider) versus draft #4259/#4647. It belongs to the provider area. |
14 changes: 9 additions & 5 deletions devlog/_plan/260927_release_train_4/gui-ux/_handoff.md
Original file line number Diff line number Diff line change
@@ -1,9 +1,13 @@
# GUI UX lane handoff — 2026-09-28 KST
# GUI UX lane handoff — final (2026-09-28 KST)

Stopped at the coordinator's request. Dedicated checkout: `/Users/jun/.codex/worktrees/t4-gui-ux/opencodex`, branch `codex/t4-gui-ux-model-settings`, latest work commit `05ff0a8372` (parent roadmap commit `a60b078cda`); **local only, not pushed**. No lane PR was created or merged, and no GitHub issue/PR comment or closure was made. The seven source PRs and three issues were open at intake; refresh their states before acting. Last verified `origin/dev` was `24b2f39b77`, #6058 head `0798999c6f`.
The lane is closed for release train 4. #6058's routed-model capability editor landed through #6105 (merge `3401e1ee73`), and #6058 was closed with credit. Every other assigned PR and issue has an English disposition comment and stays open. The table is in `040_dispositions.md`.

Docs-first `wp0` PABCD cycle is complete. The session `01a0e337-f3a7-7380-82dd-4a66bb2da2fd` is at **A** for `wp1` (#6058). The independent A reviewer `01a0e372-f380-7e11-bf2b-661afd132a50` was shut down before a verdict; do not treat it as approval. The `wp1` architect reflected on the amended `010_model_settings.md` and returned ALIGNED. The roadmap, PR/issue disposition and UX state design are in numbered files here. Product code is still unchanged from `dev`.
For the next train:

The before-change live GUI screenshot is in ignored `.tmp/gui-ux/before-models-desktop.png`. Its synthetic proxy was run from direct `startServer` with isolated OpenCodex/Codex homes and client integrations OFF. An earlier CLI-start attempt rewrote the real Grok managed endpoint to test port 18761; it was stopped and the endpoint restored to the running user proxy on 10100, with readback verification. Details are in ignored `.tmp/gui-ux/qa-isolation-incident.md`. **Use only the direct server entrypoint for future local QA.** Unreleased #6058 and #4649 security notes are in ignored `.tmp/gui-ux/*-security-review.md`, not tracked docs.
- **#4932.** Start from the audited plan in `020_combo_sidecar.md`, including its four A findings. The next step is B on a fresh branch from `dev`.
- **#5617.** Decide the precedence and the migration and rollback between global and provider visibility before any UI work, then wait for #6106 to settle `Providers.tsx`.
- **Registry enrichment.** Caching it per provider in `listManagementModelRows` was declined on #6105 as a trivial-priority refactor. Revisit it if `/api/models` latency shows up on large rosters.

Next: fetch current `dev` and #6058; obtain a fresh `gpt-6-sol` independent A verdict on `010_model_settings.md` and its owning code. If approved, carry #6058 onto this lane branch with contributor credit, repair the documented validation, persistence and dialog recovery defects, then verify focused tests/typecheck/GUI lint/build and real desktop/narrow/long-locale click flows. Run `test:changed` or a full suite only in a verification-only checkout of the exact code commit at `/private/tmp/t4-gui-ux-verify` because tests under `~/.codex` can hit protected cleanup paths. Screenshots for a code PR go to `pr-assets` by SHA, never the code branch. Required exact-head CI and post-merge `dev` CI are still outstanding. Later phases #4932, #5617 and the deferred PR/issue dispositions remain unstarted.
For QA, use `.tmp/gui-ux/qa-server.ts` (direct `startServer`, refuses non-isolated homes) with `.tmp/gui-ux/qa-setup.sh`, and Playwright-core in `/private/tmp/t4-gui-ux-pw` driving the system Chrome. `.tmp/gui-ux/real-home-sentinel.sh` hashes the real-home client configs before and after a run; they were unchanged through this lane's QA.

The Codexclaw FSM for coordinator session `01a0e37e-639d-7693-a2bc-5e4df49fa656` closed `wp1` at D. `wp2` was stopped at A by the scope change, with no B, C or D recorded.
Loading