Skip to content

fix(catalog): honor disabled vision sidecar for image input - #7126

Closed
oocheol wants to merge 3 commits into
lidge-jun:devfrom
oocheol:codex/fix-disabled-vision-catalog
Closed

oocheol wants to merge 3 commits into
lidge-jun:devfrom
oocheol:codex/fix-disabled-vision-catalog

Conversation

@oocheol

@oocheol oocheol commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

Summary

With visionSidecar.enabled: false, text-only models still advertised image attachments even though request-time planning disabled the sidecar. Cached synthetic modalities and concurrent gathers could preserve the wrong setting, and changing the setting in a running proxy did not update the catalog/cache Codex reads.

  • Keep provider discovery/cache evidence free of synthetic sidecar modalities, then project attachment metadata using the effective setting captured before discovery. Include that setting in the gather flight key.
  • Apply the setting to discovered/configured rows, custom rows and synthesized combo members. Preserve native image support, explicit custom overrides and the provider-only helper's default-enabled behavior.
  • After an effective Vision switch change through PUT /api/sidecar-settings, persist first and invoke the existing bounded catalog-only management funnel to publish both managed catalog and Codex cache. Unchanged effective settings, unrelated partial updates and rejected inputs do not refresh. The response exposes incomplete publication as catalogRefreshPending; it does not claim native-client reload.
  • Add 20 catalog regressions and 11 management regressions, including actual catalog/cache writes in disposable homes. Synchronize all eight sidecar documentation locales and the catalog/management contracts.

Closes #7086

Verification

Head: a1f1190547b4544ae81299590fbc0cdf268b52ec. Base: 7640a942b35044c974e534b84d5a8049f4a8ae9a (dev, refreshed before submission). The maintainer's a32496dd1 fix-up and existing authorship are preserved. The following receipts were measured on Windows with Bun 1.4.3 against the final source changes.

  • New management regression file: 11 passed / 0 failed, 45 assertions. Before the route fix, the same file produced 10 failures / 1 pass. The live-toggle case exercises the real management convergence path and reads both opencodex-catalog.json and models_cache.json after disabling and re-enabling.
  • Scoped changed-mode validation: 355 passed / 0 failed across 17 files, 1,739 assertions, 237.30 seconds. This includes the 20 catalog regressions, all 11 new management cases, and existing sidecar, CLI, relay, Claude and web-search consumers. The command took time to select its tests but completed successfully; it was not cancelled.
$relatedTests = @(rg -l 'sidecar-settings' tests --glob '*.test.ts')
$extraTests = @('tests/codex-integration/catalog-vision-sidecar-disabled.test.ts', 'tests/codex-integration/catalog-vision-sidecar-modalities.test.ts')
$allTests = @($relatedTests + $extraTests | Sort-Object -Unique)
bun run test:changed --parallel=1 --timeout 45000 @allTests
  • Explicit layout and core-boundary guards: bun scripts/test.ts --parallel=1 --timeout 45000 tests/test-layout.test.ts tests/test-layout-tooling.test.ts tests/lab/core-lab-boundary.test.ts: 52 passed / 0 failed, 659 assertions. Combined with the scoped run: 407 passed across 20 distinct files.
  • bun run typecheck, bun run structure:check, bun scripts/file-size-ratchet.ts, bun run privacy:scan, and git diff --check: passed. Both required test-layout maps are updated and no line caps were raised.
  • cd docs-site; bun run build: passed; 577 pages / 81,200 internal links. Dependencies were already installed using frozen lockfiles; lockfiles are unchanged.

Full-suite resource exception: this is a shared Windows host, and the bounded 17-file changed-mode run alone took nearly four minutes. The full 2,327-file suite was not run. Hosted checks on prior heads are not validation of this head; new CI and independent review remain pending. All new integration writes use disposable test homes and static model fixtures; no live provider inference, user configuration change, or rendered native-client reload was tested.

Review follow-up

  • Codex P1: verified against current code and fixed by post-save catalog/cache convergence. Tests cover default/explicit switch transitions, no-op/partial/invalid writes, bounded request construction, publication failure reporting, and real on-disk modalities.
  • Codex P2: verified and fixed in French, Japanese, Russian, Turkish and Traditional Chinese. All eight locales also document the new settings-refresh response.
  • CodeRabbit reported no actionable findings at a32496dd1. That review predates this follow-up and is not approval of the new management-route change.

Checklist

  • Scope stays focused and avoids unrelated cleanup.
  • Docs or release notes were updated when needed.
  • Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults. The follow-up changes a management API write path and therefore still needs the explicit maintainer review required by repository policy. It reuses the existing catalog-only authority; authentication, credentials, provider destinations and workflow policy are unchanged. Privacy scan passed.

Review readiness checklist

This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:

  • Required local validation passed; commands, results, and any full-suite exception are documented.
  • I pushed my PR to a recent dev commit (at most 10 behind; a maintainer may still ask for the exact tip before merge).
  • I resolved all correct Codex and CodeRabbit findings.
  • My PR is ready for review.

Co-authored-by: 정우철 oocheol@naver.com

Summary by CodeRabbit

  • Bug Fixes
    • Catalog image-input listings now reflect whether Vision sidecar support is enabled, including for custom models and combo entries. Native image support and explicitly configured image modalities remain listed.
    • Catalog listings stay consistent when sidecar settings change, including when cached model data is reused or a fetch fails.
    • Changing the effective Vision setting now refreshes the managed catalog and cache after saving. If refresh is incomplete, the saved setting remains in place and the response reports that refresh is pending.
  • Documentation
    • Clarified how sidecar settings affect catalog image-input listings, combo requirements, and catalog refresh status.

@coderabbitai

coderabbitai Bot commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: lidge-jun/opencodex/.coderabbit.yaml
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: 12c41b41-b76a-443f-b8c6-332668fc1c12
📥 Commits

Reviewing files that changed from the base of the PR and between a32496d and a1f1190.

📒 Files selected for processing: 14
  • docs-site/src/content/docs/fr/guides/sidecars.md
  • docs-site/src/content/docs/guides/sidecars.md
  • docs-site/src/content/docs/ja/guides/sidecars.md
  • docs-site/src/content/docs/ko/guides/sidecars.md
  • docs-site/src/content/docs/ru/guides/sidecars.md
  • docs-site/src/content/docs/tr/guides/sidecars.md
  • docs-site/src/content/docs/zh-cn/guides/sidecars.md
  • docs-site/src/content/docs/zh-tw/guides/sidecars.md
  • scripts/test-layout/layout.json
  • src/server/management/config-routes.ts
  • structure/catalog.md
  • structure/gui-and-management-api.md
  • tests/fixtures/test-layout-expected.json
  • tests/vision/sidecar-settings-vision-catalog.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

📝 Walkthrough

Catalog image-input metadata now follows the effective Vision sidecar setting across provider, cached, custom, and combo models. A changed setting triggers catalog convergence after persistence. Tests and documentation cover these behaviors and preserve native and explicitly configured image modalities.

Changes: Vision sidecar catalog behavior

Layer / File(s) Summary
Separate provider evidence from sidecar projection
src/codex/catalog/model-hints.ts, src/codex/catalog/provider-models.ts
Provider hint helpers can defer synthetic sidecar image input. Provider discovery applies sidecar input when it projects returned models.
Apply the captured setting across catalog rows
src/codex/catalog/gather-capture.ts, src/codex/catalog/routed-gather.ts, src/codex/catalog/combo-member.ts, src/codex/catalog/model-visibility.ts, tests/codex-integration/catalog-vision-sidecar-disabled.test.ts, tests/fixtures/test-layout-expected.json, scripts/test-layout/layout.json, docs-site/src/content/docs/*/guides/sidecars.md, structure/catalog.md
Catalog gathering captures the effective setting, applies it to provider, custom, metadata, and combo rows, and includes it in gather identity. Tests cover setting changes, cached models, and combo behavior. Documentation describes the catalog rules.
Converge catalog after settings persist
src/server/management/config-routes.ts, tests/vision/sidecar-settings-vision-catalog.test.ts, structure/gui-and-management-api.md
The management route converges the catalog after persisting an effective Vision setting change. Its response reports whether refresh work remains pending. Tests cover persistence order, unchanged settings, invalid writes, and incomplete refreshes.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix · Severity of issue fixed: Medium

Sequence Diagram(s)

sequenceDiagram
  participant SettingsRoute
  participant SettingsStore
  participant CodexCatalog
  SettingsRoute->>SettingsStore: persist changed Vision setting
  SettingsRoute->>CodexCatalog: converge catalog after persistence
  CodexCatalog-->>SettingsRoute: refresh outcome
  SettingsRoute-->>SettingsRoute: report catalogRefreshPending
Loading
Merge Risk: ⚪ Minimal · up to a1f11

The change appears mergeable after normal checks; no concrete outstanding issue is established by the supplied evidence.

Pre-merge checks: Passed 5
Passed Passed checks: 5 passed
Check name Status Explanation
Linked Issues check Passed Issue #7086 requires catalog rows to omit synthetic image support when visionSidecar.enabled is false, while preserving native image support, explicit modalities, and enabled/default sidecar adverti…
Out of Scope Changes check Passed The changed catalog code, gather-key separation, cache tests, management convergence, and catalogRefreshPending response all support the catalog contract in issue #7086. The management route change …
Docstring Coverage Passed Docstring coverage is 81.82% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 22 functions across 9 files. (12 skipped: 1…
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly and concisely describes the main change: catalog image-input metadata now respects a disabled Vision sidecar.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

@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 Oct 15, 2026
@github-actions

github-actions Bot commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

⏳ DRAFT

  • review readiness checklist open (3/4 boxes ticked).

What to do

  • Tick all four boxes in the PR description once you're done (currently 3/4).

Review readiness checklist

  • ✅ Required local validation passed; commands, results, and any full-suite exception are documented.
  • ✅ I pushed my PR to a recent dev commit (at most 10 behind; a maintainer may still ask for the exact tip before merge).
  • ✅ I resolved all correct Codex and CodeRabbit findings.
  • ⬜ My PR is ready for review.

3/4 boxes ticked.

This PR stays in draft until every box above is ticked.

@lidge-jun
lidge-jun marked this pull request as ready for review October 15, 2026 07:28
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 15, 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-10-15T07:36:11.183836Z a32496d 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.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a32496dd14

ℹ️ 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".

Comment thread src/codex/catalog/routed-gather.ts
Comment thread docs-site/src/content/docs/guides/sidecars.md
@lidge-jun

Copy link
Copy Markdown
Owner

Being carried into #7220 with your authorship preserved (own commit + Co-authored-by); this PR will be closed when the bundle lands on dev.

lidge-jun added a commit that referenced this pull request Oct 16, 2026
… ids and auto-review override (#7115, #7126, #7173, #6867) (#7220)

Carried from #7115, #7126, #7173, #6867 (one commit per source PR, original authors preserved).

Remote exact-head validation (hosted GitHub Actions stalled; local tests not run by maintainer rule): union candidate codex/prr-integ-6 (368006f, tree 27fdc37) on mini (Windows) and lidge (Linux), typecheck + full suite, compared with base-dev 4ae4936 on each host.

Co-authored-by: 정우철 <oocheol@naver.com>
Co-authored-by: Rafael Moreira <rrmlima@gmail.com>
Co-authored-by: sechmachine <97589681+sechmachine727@users.noreply.github.com>
@lidge-jun

Copy link
Copy Markdown
Owner

Landed on dev in f6e6f1b via #7220 (your change is its own commit in that bundle, and the squash keeps your authorship as Co-authored-by). Thank you!

@lidge-jun lidge-jun closed this Oct 16, 2026
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.

2 participants