refactor(vision): split planning and image rewriting out of the vision index (split S06 L1/2)

[lidge-jun]

Summary

• Pure move: src/vision/index.ts (667 lines) splits into src/vision/plan.ts (200: sidecar planning — timeout/cap resolution, backend and model resolution, planVisionSidecar) and src/vision/image-rewrite.ts (106: image-part detection, in-place stripping and raw-body description sync), leaving a 381-line index that owns the description cache and describeImagesInPlace and re-exports every one of the 38 previously exported names. All 24 existing importers keep their ../vision path.
• Why: 667-line file over the 400-line module limit; planning, rewriting and execution/caching are separate concerns with a strict one-way dependency (index → plan → image-rewrite). Zero behavior change — the LRU cache, its limits and eviction hooks stay single-owned in the index.
• Plan and evidence: devlog/_plan/260905_now_split_train/190_vision_index.md; rules 003_parent_decisions.md (PURE-MOVE-SIZE-01, S06-ORACLE-01).

Stack (S06 media — independent layers, each based on dev; no cascade between them):

#	PR	Branch	Base	Review focus
2	TBD	codex/split-images-artifacts	dev	images/artifacts
1	this PR	codex/split-vision-index ← you are here	dev	plan / image-rewrite leaves

Base: dev. Review this PR's diff only (4 files, +338/−305; non-move diff: 11 leaf import lines, the replaced 40-line header, 3 export modifiers, 13 test lines). Move-aware view: git diff --color-moved=dimmed-zebra dev...HEAD.

Verification

• bun run typecheck → exit 0
• Guards: tests/lab/core-lab-boundary.test.ts tests/codex-integration/codex-history-reachability.test.ts tests/windows/windows-popup-fix.test.ts → 27 pass / 0 fail (the recursive source walkers pick up both leaves automatically)
• Focused tests/vision + 6 importer files → 254 pass / 2 fail; the 2 failures (sidecar-settings-vision-filter case 10, vision-reasoning-contract management rows) reproduce identically on pristine dev when tests/vision runs as one batch and pass 19/0 when run alone — order-dependent and pre-existing, not from this change.
• Red-drives, then restored: carriesImages excluding user fails vision-fail-closed.test.ts:20; losing explicit-zero in resolveMaxDescriptionsPerTurn fails vision-cache.test.ts:134; a runtime Lab import in image-rewrite.ts fails the transitive boundary guard with the full chain.
• bun run privacy:scan → passed
• New test: seam identity for resolveMaxDescriptionsPerTurn and stripImagesInPlace via both paths; leaves have no back-edge to index/plan.
• Full suite on the remote CI host (lidge) at this exact SHA: recorded in the devlog doc.

Checklist

• Scope stays focused and avoids unrelated cleanup.
• Docs or release notes were updated when needed (devlog unit records the layer; no user-facing change).
• Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults (sidecar auth resolution moved byte-for-byte inside planVisionSidecar; no auth reads added at module load).

Summary by CodeRabbit

• New Features

  • Improved vision processing for image-containing requests, including image descriptions and clear handling of omitted or empty images.
  • Added configurable vision planning for OpenAI, Anthropic, and routed backends, including model, timeout, and per-turn description settings.

• Refactor

  • Reorganized vision functionality into dedicated planning and image-handling modules without changing the public vision interface.

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-09-05T01:57:58.958921Z	51f5a82	PR opened

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

[github-actions]

✅ Deterministic PR hygiene checks passed.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Team

Run ID: 65b45d87-5fe7-4d2b-bed9-20ccffd2ea84

📥 Commits

Reviewing files that changed from the base of the PR and between 24cc558 and 51f5a82.

📒 Files selected for processing (4)

• src/vision/image-rewrite.ts
• src/vision/index.ts
• src/vision/plan.ts
• tests/vision/vision-cache.test.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review.

---

📝 Walkthrough

Walkthrough

The change extracts vision sidecar planning and image rewriting from src/vision/index.ts into dedicated modules. It adds raw-body image synchronization, image stripping with budget accounting, and boundary tests for exports and dependency direction.

Changes

Vision image rewriting

Layer / File(s)	Summary
Image rewriting and budget handling src/vision/image-rewrite.ts	Adds image-capable role detection, image-to-caption rewriting, omitted-image markers, raw-body synchronization, and translator-budget accounting when images are stripped.

Vision sidecar planning

Layer / File(s)	Summary
Vision sidecar planning src/vision/plan.ts	Moves provider, backend, model, timeout, eligibility, and sidecar-plan resolution into a dedicated module. It supports OpenAI, Anthropic, and routed planning paths.

Vision boundary exports and validation

Layer / File(s)	Summary
Vision boundary exports and validation src/vision/index.ts, tests/vision/vision-cache.test.ts	Updates the vision boundary to re-export the extracted APIs and verifies export identity and module dependency direction.

Estimated code review effort: 4 (Complex) | ~45 minutes

Merge Risk: 🟡 Moderate · up to 51f5a

The module split may break existing consumers that import several image-rewrite helpers through src/vision. Restore those boundary exports or explicitly accept the compatibility break before merging.

Sequence Diagram(s)

sequenceDiagram
  participant ParsedRequest
  participant VisionPlanner
  participant VisionSidecar
  participant ImageRewriter
  ParsedRequest->>VisionPlanner: inspect model, provider, and image eligibility
  VisionPlanner->>VisionSidecar: resolve backend and description settings
  VisionSidecar-->>VisionPlanner: return vision plan
  ParsedRequest->>ImageRewriter: strip or synchronize image content
  ImageRewriter-->>ParsedRequest: return rewritten request content

Loading

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 20 functions across 4 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: splitting vision planning and image rewriting out of src/vision/index.ts. The refactor scope is specific and relevant to the pull request obj…
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

• Fix all pre-merge checks with AI

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch codex/split-vision-index

---

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

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[lidge-jun]

리뷰 · 우선순위 42 / 80

이 PR은 src/vision/index.ts가 667줄로 모듈 한도(400줄)를 넘긴 것을, 동작 변경 없이 관심사별로 쪼개는 pure-move다. 계획·설정 해석은 src/vision/plan.ts(약 200줄: 타임아웃/캡, 백엔드·모델 결정, planVisionSidecar), 이미지 파트 탐지·제거·raw body 설명 동기화는 src/vision/image-rewrite.ts(약 106줄: carriesImages, syncRawBodyImageDescriptions, stripImagesInPlace), 설명 캐시와 describeImagesInPlace 실행은 index.ts에 남겨 약 381줄로 내린다. 기존에 바깥으로 나가던 export 이름은 index에서 재export해서, ../vision 경로를 쓰는 importer는 경로를 바꿀 필요가 없다. 베이스는 dev이고 S06 media 스택의 L1/2 레이어(vision index split)로, images/artifacts 쪽 다른 브랜치와는 cascade 없이 독립이라고 본문에 적혀 있다.

지금 dev(HEAD 24cc558d5, package 2.43.0)의 src/vision/index.ts는 실제로 667줄이고, planVisionSidecar / resolveMaxDescriptionsPerTurn / describeImagesInPlace / stripImagesInPlace가 한 파일에 섞여 있다. PR diff(+338/−305, 파일 4개)는 그 덩어리를 옮기는 형태고, 비이동 diff는 leaf import·헤더·export 수식어·테스트 십수 줄 수준이라고 본문이 주장한다. 의존 방향은 index → plan → image-rewrite 한 방향으로 잡혀 있고, plan이 carriesImages를 image-rewrite에서 가져가 메시지에 이미지가 있는지만 본다. 캐시 LRU·한도·evict 훅은 index에 단일 소유로 남는다고 해서, split 때문에 캐시 키가 두 곳으로 갈라질 위험은 설계상 피한 편이다. 검증도 typecheck·boundary guard·vision 포커스 테스트·privacy scan·seam identity 테스트를 적어 두었고, 배치에서만 깨지는 기존 실패 2건은 pristine dev에서도 같다고 명시했다.

메인테이너 관점에서 이 레이어는 “지금 핫픽스 트레인을 막을 버그”가 아니라 모듈 한도·가독성·이후 S06 작업을 위한 구조 정리에 가깝다. #3561/#3562 OAuth/Kiro 캐리나 Windows suite, combo 바운디드 스택과는 파일이 겹치지 않는다. types/config 스플릿 close-don't-rebase 대상도 아니다. 작성자가 유지보수자 본인(lidge-jun)이고 chore 라벨이라, 리뷰 포인트는 동작 동치·순환 import·export 표면 유지·남은 index 줄 수다. index가 381줄이면 400줄 한도 안이고, plan 200 / image-rewrite 106도 한도 아래다. 순수 이동이 맞다면 머지 비용이 낮고, S06 다음 레이어(images/artifacts) 전에 깔아 두는 편이 맞다.

다만 pure-move라도 리뷰어가 볼 구멍은 있다. 첫째, resolveMaxDescriptionsPerTurn 등 런타임 config permissive 규칙(0은 의도적, 잘못된 값은 기본값)이 이동 중에 미묘하게 바뀌지 않았는지 seam 테스트가 양쪽 경로로 같음만 보는지, 경계값(0 / 음수 / 비정수)까지 보는지를 확인해야 한다. 둘째, image-rewrite가 index/plan을 import하지 않는지(역방향 금지)는 새 테스트가 있다고 하니 그 assert가 CI에서 실제로 도는지 보면 된다. 셋째, syncRawBodyImageDescriptions는 Responses 패스스루 _rawBody와 context.messages를 맞추는 민감 경로라, 이동 중 호출 순서·description 커서 소비가 바뀌면 텍스트-only 업스트림에 원본 픽셀이 다시 나갈 수 있다. 넷째, 본문이 말한 기존 flaky 2건은 이 PR 책임이 아니어도, 비전 배치가 기본 CI 게이트면 머지 후 소음이 남을 수 있다.

src/vision/index.ts (dev 667줄 → PR 약 381줄) - 모듈 한도(400) 초과가 이 스플릿의 직접 동기다. 캐시·describeImagesInPlace만 남기는 방향은 타당하다.
src/vision/plan.ts - 타임아웃/캡/백엔드·모델/planVisionSidecar 이동. importer는 index 재export에 의존하므로 공개 경로가 깨지면 여기가 아니라 index 재export 누락이다.
src/vision/image-rewrite.ts - carriesImages / strip / raw-body sync. plan→image-rewrite 단방향이 깨지면 순환이 생긴다.
tests/vision/vision-cache.test.ts - seam identity·leaf back-edge 가드가 이 PR의 실질 회귀망이다. 경계값·호출 순서까지 커버하는지는 확인 필요.
본문 flaky 2건 (sidecar-settings-vision-filter case 10, vision-reasoning-contract) - pristine dev에서도 배치 실패라면 이 PR과 분리해 추적하는 편이 맞다.

메인테이너의 판단이 필요한 지점

• S06 L1/2를 지금 머지해 images/artifacts 레이어 전에 깔지, 다른 핫픽스 뒤에 둘지.
• pure-move로 보고 squash/머지 트레인에 태울지, git diff --color-moved로 한 번 더 사람 확인 후 받을지.
• 기존 vision 배치 flaky 2건을 이번 머지 조건에 넣을지, 별 이슈로 남을지.
• index 재export 목록이 “기존 38 export 전부”인지, 빠진 심볼이 있으면 컴파일/테스트가 잡아주는 구조인지.

너의 추천
라벨 chore 맞다. 동작 변경 없는 모듈 스플릿이고 dev 한도·S06 계획과도 맞다. 머지 전에 (1) git diff --color-moved=dimmed-zebra dev...HEAD로 비이동 줄이 본문 주장(11+40+3+13) 수준인지 한 번 보고, (2) seam 테스트가 0/잘못된 값/stripImagesInPlace 경로를 양쪽 import로 통과하는지 확인한 뒤 받는 걸 추천한다. 우선순위 42 — 구조 개선·한도 준수·트래인 비충돌, 사용자 임팩트는 없음. 플래키 2건은 별도 이슈/후속으로 두자.

이 댓글은 grok-bot이 작성했습니다

[Ingwannu]

Approved on exact head 51f5a82d7c6ff3cc3a2df1a08716fa5eff1e67b1.

I compared the complete tests/vision batch under isolated homes on this head and on current dev (526d4bf644038deb0c89852597f4551773d46198). The PR produced 171 pass / 2 fail; pristine dev produced 170 pass / the same 2 fail, with the same assertions in vision-reasoning-contract and sidecar-settings-vision-filter case 10. The one extra passing test is this PRs seam/identity guard, so those failures are pre-existing order-dependent baseline noise rather than a regression from the split. Typecheck also passes.

The moved planning and image-rewrite logic retains the existing public exports and one-way dependency boundary. Required macOS jobs are still pending and must finish green before merge. Non-blocking cleanup: git diff --check reports one extra blank line at EOF in src/vision/index.ts:381.

[lidge-jun]

Landed via #3668 at 6585e6a

[lidge-jun]

Superseded by aggregate PR #3668, admin-merged into dev as 6585e6a after exact-head CI and tested-tree verification. This original PR was not individually merged; its rebased content and historical records were consolidated in #3668. The original branch is preserved. Further unimplemented debt layers remain deferred.