diff --git a/devlog/_plan/260809_vision_sidecar_model_filter/000_plan.md b/devlog/_fin/260809_vision_sidecar_model_filter/000_plan.md similarity index 100% rename from devlog/_plan/260809_vision_sidecar_model_filter/000_plan.md rename to devlog/_fin/260809_vision_sidecar_model_filter/000_plan.md diff --git a/devlog/_plan/260809_vision_sidecar_model_filter/001_capability_signal_inventory.md b/devlog/_fin/260809_vision_sidecar_model_filter/001_capability_signal_inventory.md similarity index 100% rename from devlog/_plan/260809_vision_sidecar_model_filter/001_capability_signal_inventory.md rename to devlog/_fin/260809_vision_sidecar_model_filter/001_capability_signal_inventory.md diff --git a/devlog/_plan/260809_vision_sidecar_model_filter/002_audit_synthesis.md b/devlog/_fin/260809_vision_sidecar_model_filter/002_audit_synthesis.md similarity index 100% rename from devlog/_plan/260809_vision_sidecar_model_filter/002_audit_synthesis.md rename to devlog/_fin/260809_vision_sidecar_model_filter/002_audit_synthesis.md diff --git a/devlog/_plan/260809_vision_sidecar_model_filter/010_vision_eligibility_core.md b/devlog/_fin/260809_vision_sidecar_model_filter/010_vision_eligibility_core.md similarity index 100% rename from devlog/_plan/260809_vision_sidecar_model_filter/010_vision_eligibility_core.md rename to devlog/_fin/260809_vision_sidecar_model_filter/010_vision_eligibility_core.md diff --git a/devlog/_plan/260809_vision_sidecar_model_filter/020_management_api_allowed_models.md b/devlog/_fin/260809_vision_sidecar_model_filter/020_management_api_allowed_models.md similarity index 100% rename from devlog/_plan/260809_vision_sidecar_model_filter/020_management_api_allowed_models.md rename to devlog/_fin/260809_vision_sidecar_model_filter/020_management_api_allowed_models.md diff --git a/devlog/_plan/260809_vision_sidecar_model_filter/030_dashboard_vision_card.md b/devlog/_fin/260809_vision_sidecar_model_filter/030_dashboard_vision_card.md similarity index 100% rename from devlog/_plan/260809_vision_sidecar_model_filter/030_dashboard_vision_card.md rename to devlog/_fin/260809_vision_sidecar_model_filter/030_dashboard_vision_card.md diff --git a/devlog/_plan/260809_vision_sidecar_model_filter/040_stack_publication.md b/devlog/_fin/260809_vision_sidecar_model_filter/040_stack_publication.md similarity index 100% rename from devlog/_plan/260809_vision_sidecar_model_filter/040_stack_publication.md rename to devlog/_fin/260809_vision_sidecar_model_filter/040_stack_publication.md diff --git a/devlog/_fin/260809_vision_sidecar_model_filter/050_stack_landing.md b/devlog/_fin/260809_vision_sidecar_model_filter/050_stack_landing.md new file mode 100644 index 0000000000..6cc25aca4e --- /dev/null +++ b/devlog/_fin/260809_vision_sidecar_model_filter/050_stack_landing.md @@ -0,0 +1,104 @@ +# 050 — phase 5: land the stack on `dev` + +No production code of its own. This phase takes the three published layers from +`040_stack_publication.md` and merges them into `dev`, bottom-up, one PABCD cycle +per layer. It also carries the automated-review debt that arrived after the stack +was opened. + +## Authorization + +The publication phase deliberately stopped at "PRs open, merging not authorized". +The user has since granted merge authority explicitly ("dev에 머지해"), so this +phase exists as a separate goal rather than an extension of the previous one. The +boundary is recorded here because the earlier goalplan states the opposite. + +## Why bottom-up, one at a time + +``` +#1328 card → base #1327 ──┐ +#1327 api → base #1326 ──┼── each retarget to dev happens only AFTER its parent lands +#1326 core → base dev ──┘ +``` + +A stacked child shows its parent's commits in the diff until the parent lands. +Merging the parent and then retargeting the child to `dev` collapses the child's +diff to its own layer, which is what makes the second review meaningful. Merging +top-down, or retargeting before the parent lands, would push all three layers +through one review surface and defeat the split. + +`enforce-target` skips the wrong-base gate for children of an OPEN parent. Once +the parent merges, the child must be retargeted to `dev` or the gate turns red — +so the retarget is part of the merge step, not a follow-up. + +## Review debt (gate for this phase) + +Eight inline findings landed on the stack after publication: five from the Codex +reviewer, three from CodeRabbit. They share one root cause worth naming, because +it decides whether they are separate bugs or one design defect: + +> An option is emitted as a bare model **id**, and the provider identity that +> proved it eligible is discarded. Every consumer downstream then re-derives a +> provider — and each one re-derives it differently. + +That single discard produces: baselines inserted with no provider to check +`noVisionModels` against (`eligibility.ts:127-131`), a non-selected +`adapter: "anthropic"` provider's unique ids offered but dispatched to the +selected OAuth endpoint (`eligibility.ts:100`), a bare-id early return that skips +the authoritative backend probe (`vision-sidecar-options.ts:82`), and a GUI that +drops the server-supplied `backend` and re-infers it from `/api/models` +(`dashboard-overview-sections.tsx:302`). + +### Triage verdict (independent pass, current head `51bfc78`) + +Seven of the eight are VALID against current code; five block the merge. + +| Finding | Verdict | Layer | +|---|---|---| +| `eligibility.ts:64` consumer membership read from raw config, not registry-enriched | VALID — `enrichProviderFromRegistry` backfills `noVisionModels` that this predicate never sees, and the enriched catalog then force-adds `"image"` to exactly those rows | 1, blocking | +| `eligibility.ts:100` every `adapter: "anthropic"` provider is treated as the executor | VALID — dispatch uses only `findAnthropicVisionProvider` (first OAuth provider); a key-auth row like `umans` is offered but unreachable | 1, blocking | +| `eligibility.ts:131` baselines inserted with no eligibility check | VALID — a baseline listed in `noVisionModels` is still offered | 1, P2 | +| `eligibility.ts:78` bare-id collision claims native capability | VALID — a non-native row declaring `["text"]` is overridden by the native table | 1, P2 | +| `vision-sidecar-options.ts:82` first matching row short-circuits the backend probe | VALID — a custom `o3-mini` row declaring image lets a text-only model past the write gate | 2, blocking | +| `vision-sidecar-options.ts:45` non-executor anthropic rows suggested | VALID, but the same defect as `eligibility.ts:100` — one fix, not two | 2, blocking | +| `config-routes.ts:396`/`:501` grandfathers `gpt-5.4-mini` under an anthropic backend | VALID — runtime default is `claude-sonnet-5`; both response paths repeat it | 2, blocking | +| `dashboard-overview-sections.tsx:302` GUI re-infers the backend it was given | VALID — `visionModelOptions` maps `backend` away, then `sidecarBackendForModel` guesses `openai` | 3, blocking | +| CodeRabbit doc finding on 020/030 (no-executor fallback) | INVALID against the approved design — `020` explicitly specifies baselines as the catalog-outage floor, and the code implements that. Recorded, not actioned. | — | + +Findings are triaged before any merge, per layer: + +- A finding against layer N's own code is fixed on layer N's branch **before** + that layer merges. Fixing it later means shipping a known defect into `dev` and + reviewing the fix without the context that produced it. +- A finding that is INVALID against current code is closed with the concrete + reason, not silently ignored. +- A finding whose fix would change the layer split (a new module, a schema + change reaching all three layers) becomes an appended work-phase rather than a + quiet in-place rewrite. + +## Sequence per layer + +1. Confirm `gh pr checks ` has no `fail` row on the CURRENT head. A cancelled + shard is not a pass: rerun it and wait for the real verdict. Both #1326 and + #1327 hit runner cancellations at 15m on the first pass, which reported as + `fail` and had to be reruns rather than debugged. +2. Fold that layer's verified findings, push, and let CI settle again. +3. Merge into `dev`. +4. Retarget the child PR to `dev` and confirm its changed-file list shrank to its + own layer. +5. Record the merge commit and the post-merge `dev` tip in the ledger. + +## Rollback + +Each layer is a separate merge commit on `dev`, so a bad layer reverts alone. The +GUI layer degrades to the legacy list against an older server, and the API layer +is inert without a caller, so a revert of layers 2-3 leaves the predicate in place +with no user-visible surface. Reverting layer 1 requires reverting all three. + +## Acceptance + +- Three PRs in state `MERGED` with `dev` as the final base. +- `origin/dev` contains the devlog unit, the predicate, the API guard, and the + card restyle. +- `bun run typecheck` and `bun run test` green on the landed `dev` tip, run fresh + rather than inherited from the last PR run. +- The unit moved to `devlog/_fin/` with the merge commits recorded. diff --git a/devlog/_fin/260809_vision_sidecar_model_filter/060_outcome.md b/devlog/_fin/260809_vision_sidecar_model_filter/060_outcome.md new file mode 100644 index 0000000000..14ab400450 --- /dev/null +++ b/devlog/_fin/260809_vision_sidecar_model_filter/060_outcome.md @@ -0,0 +1,70 @@ +# 060 — outcome + +Shipped. Three layers on `dev`, bottom-up, one PABCD cycle each. + +| Layer | PR | Merge commit | What landed | +|---|---|---|---| +| 1 — eligibility predicate | #1326 | `eebd9d48f` | `src/vision/eligibility.ts`, the devlog unit | +| 2 — management API + write gate | #1327 | `d4758bc94` | options module, both routes, shared model resolver | +| 3 — dashboard card | #1328 | (this unit's close) | filtered picker, backend provenance, card shell | + +## What the user asked for, and what answers it + +**"Only models that can actually read an image."** `visionEligibleModelOptions` emits a +row only when an executor can reach it and no source proves it blind. The trap that +shaped the design is in `000_plan.md`: `noVisionModels` marks models the proxy describes +FOR, and the catalog deliberately ADDS `"image"` to exactly those rows, so advertised +modalities alone would have selected blind describers. + +**"gpt-5.6-luna and claude-haiku-4-5 must always be there."** Each enabled side's +baseline is inserted before catalog candidates and survives a catalog outage, an empty +catalog, and a fresh install with no providers. It is withheld in exactly one case: the +provider explicitly lists it in `noVisionModels`. Silence never removes it. + +**"Show the allowed list, in the delegation card's shape, no i18n indirection, +compact reasoning."** Layer 3, with the web-search card given the same shell so the +two read as one row. + +## What review changed + +Eight automated findings arrived after publication; seven were real. They shared one +root cause, recorded in `050_stack_landing.md`: an option was emitted as a bare model id +and the provider identity that proved it eligible was discarded, so every consumer +re-derived a provider and each derived it differently. + +Four independent audit rounds ran on top of that, and three of them found something the +implementation had missed: + +- Layer 1 still offered a canonical Anthropic row when no executor was resolvable, so a + key-auth provider — which `findAnthropicVisionProvider` never returns — could be picked + and then fail at describe time. Fixed in `0ac7552be`. +- Layer 2's defaulted parameter re-read the OAuth account store up to four times per + response, because the no-executor case passes an explicit `undefined`. Fixed in + `6196fc5cd`. +- Layer 3 dropped the persisted backend on the grandfathered entry (`c0e651285`), and + collapsed "no `visionModels` key" with "`visionModels: []`", which let a current + server's authoritative empty answer be replaced by the unfiltered catalog — + re-offering the exact text-only rows this unit removes (`21c7157f3`). + +The last one is the one worth remembering: the feature would have shipped with a path +that quietly undid it, and only an end-to-end pass over the merged tree saw it. Per-layer +review had signed off on both halves separately. + +## Verification + +- `bun run typecheck` — exit 0 +- `bun run lint:gui` — clean +- `bun test tests/vision-eligibility.test.ts tests/sidecar-settings-vision-filter.test.ts tests/vision-reasoning-contract.test.ts ./gui/tests/vision-model-options.test.ts` — 42+ pass / 0 fail +- `bun run build:gui` — builds +- Full suite green through the pre-push gate on each layer; CI green on each PR before its merge + +Two CI flakes cost time and are worth naming so the next person does not debug them: +15-minute shard timeouts from `tests/cli-help.test.ts` hanging after a `cli-account` +error, and a bun-level `EEXIST: epoll_ctl`. Both reproduced on unrelated PR #1324, and +both passed on rerun. + +## Follow-ups, not done here + +`visionDescriberIsProvablyBlind` probes both vendor tables for an unmatched id, which is +safe only because the two tables share no bare model id. If that ever stops being true, +the probe needs the executor identity the option list already carries. diff --git a/devlog/_fin/260809_vision_sidecar_model_filter/evidence/030_sidecar_cards_matched.png b/devlog/_fin/260809_vision_sidecar_model_filter/evidence/030_sidecar_cards_matched.png new file mode 100644 index 0000000000..4a0d1e39ed Binary files /dev/null and b/devlog/_fin/260809_vision_sidecar_model_filter/evidence/030_sidecar_cards_matched.png differ diff --git a/devlog/_fin/260809_vision_sidecar_model_filter/evidence/030_vision_card_delegation_form.png b/devlog/_fin/260809_vision_sidecar_model_filter/evidence/030_vision_card_delegation_form.png new file mode 100644 index 0000000000..4f153b413e Binary files /dev/null and b/devlog/_fin/260809_vision_sidecar_model_filter/evidence/030_vision_card_delegation_form.png differ diff --git a/devlog/_fin/260809_vision_sidecar_model_filter/evidence/030_vision_card_filtered_dropdown_ko.png b/devlog/_fin/260809_vision_sidecar_model_filter/evidence/030_vision_card_filtered_dropdown_ko.png new file mode 100644 index 0000000000..a533ea5a01 Binary files /dev/null and b/devlog/_fin/260809_vision_sidecar_model_filter/evidence/030_vision_card_filtered_dropdown_ko.png differ diff --git a/gui/src/pages/dashboard-overview-sections.tsx b/gui/src/pages/dashboard-overview-sections.tsx index f62403a887..61ac28f8be 100644 --- a/gui/src/pages/dashboard-overview-sections.tsx +++ b/gui/src/pages/dashboard-overview-sections.tsx @@ -1,11 +1,10 @@ import { useEffect, useRef, useState } from "react"; import { IconAlert, IconCheck, IconInfo, IconRefresh, IconX } from "../icons"; import { Trans } from "../i18n/provider"; -import { visionReasoningLabel } from "../i18n/vision-reasoning-labels"; import { Select } from "../ui"; import { formatNamespacedModelId } from "../provider-icons"; import { navigateHash } from "../hash-routing"; -import { clampVisionReasoningToLadder, EFFORT_CAP_LEVELS, requireJson, shadowCallModelOptions, sidecarBackendForModel, updateJobLabel, visionReasoningLadder, visionReasoningOptionsFor, visionReasoningPatch } from "./dashboard-shared"; +import { clampVisionReasoningToLadder, EFFORT_CAP_LEVELS, requireJson, shadowCallModelOptions, sidecarBackendForModel, updateJobLabel, visionReasoningLadder, visionReasoningOptionsFor, visionReasoningPatch, visionSidecarBackendForModel } from "./dashboard-shared"; import { shadowSourceModelBadge } from "./shadow-call-source"; import type { useDashboardData } from "./use-dashboard-data"; @@ -239,8 +238,8 @@ export function DashboardMaintenancePanel({ d }: { d: Dash }) { export function DashboardSidecarPanels({ d }: { d: Dash }) { const { - locale, t, settings, settingsSaving, toggleCodexAutoStart, - sidecar, sidecarSaving, sidecarModels, models, saveSidecar, + t, settings, settingsSaving, toggleCodexAutoStart, + sidecar, sidecarSaving, sidecarModels, visionModels, models, saveSidecar, shadowCall, shadowCallSaving, shadowCallHelpTriggerRef, shadowCallHelpOpen, setShadowCallHelpOpen, saveShadowCall, } = d; const visionModel = sidecar?.vision.model ?? "gpt-5.4-mini"; @@ -270,9 +269,14 @@ export function DashboardSidecarPanels({ d }: { d: Dash }) {
-
-
+ {/* Both sidecar cards wear the DashboardInjectionPanel shell: the PANEL is + the flex row, copy left, controls right. */} +
+
{t("dash.webSearchSidecar")}
+
{t("dash.webSearchSidecarHint")}
+
+
{ - const ladder = visionReasoningLadder(models, model); - const reasoning = clampVisionReasoningToLadder(ladder, visionReasoning); - void saveSidecar({ vision: { model, backend: sidecarBackendForModel(models, model), reasoning } }); - }} - disabled={!sidecar || sidecarSaving} - label={t("dash.sidecarModel")} - /> - { + const ladder = visionReasoningLadder(models, model); + const reasoning = clampVisionReasoningToLadder(ladder, visionReasoning); + void saveSidecar({ vision: { model, backend: visionSidecarBackendForModel(models, visionModels, model), reasoning } }); + }} + disabled={!sidecar || sidecarSaving} + label={t("dash.sidecarModel")} + /> +