-
Notifications
You must be signed in to change notification settings - Fork 670
feat(gui): show only usable vision describers in the delegation card shape #1328
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
+436
−63
Merged
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
d6d50db
feat(gui): show only usable vision describers, in the delegation card…
lidge-jun afcac60
style(gui): give the web search sidecar the same card shell
lidge-jun 88e3823
docs(devlog): plan the vision sidecar stack landing and triage its re…
lidge-jun db5e58e
fix(gui): save the backend the server supplied with each vision option
lidge-jun c0e6512
fix(gui): carry the persisted backend onto the grandfathered vision o…
lidge-jun 21c7157
fix(gui): treat an empty vision option list as authoritative
lidge-jun 226b60f
docs(devlog): close the vision sidecar model filter unit
lidge-jun d685362
fix(gui): let the sidecar controls wrap once the card stacks
lidge-jun File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
File renamed without changes.
File renamed without changes.
File renamed without changes.
File renamed without changes.
File renamed without changes.
File renamed without changes.
File renamed without changes.
104 changes: 104 additions & 0 deletions
104
devlog/_fin/260809_vision_sidecar_model_filter/050_stack_landing.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 <n>` 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. |
70 changes: 70 additions & 0 deletions
70
devlog/_fin/260809_vision_sidecar_model_filter/060_outcome.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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. | ||
Binary file added
BIN
+133 KB
.../_fin/260809_vision_sidecar_model_filter/evidence/030_sidecar_cards_matched.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added
BIN
+134 KB
...260809_vision_sidecar_model_filter/evidence/030_vision_card_delegation_form.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added
BIN
+178 KB
...9_vision_sidecar_model_filter/evidence/030_vision_card_filtered_dropdown_ko.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Record the layer-3 merge commit.
This document states that the stack shipped, but the dashboard-card row uses
(this unit's close)instead of the actual merge commit.050_stack_landing.mdrequires each layer's merge commit to be recorded, so the final outcome is not auditable.Replace the placeholder with the layer-3 merge SHA before publishing the shipped outcome.
Proposed documentation fix
🤖 Prompt for AI Agents