feat(attachments): preserve image Read references for non-vision models - #4080
feat(attachments): preserve image Read references for non-vision models#4080me2seeks wants to merge 1 commit into
Conversation
Generated-by: Codex
Astro-Han
left a comment
There was a problem hiding this comment.
This is a synthesis of the independent blind review by @Sol-404ARE at exact head 780866ecf95bb29b4f15f31c7827a0ab6f69ce04 (base a6a08b810cac0278f2eb0fdadf38521198d6d37a, 8 files +166/-18). I verified the diff and the exact-head CI myself; the file:line findings below are from Sol's sealed review.
What I checked myself:
- Read
gh pr diff 4080(feat: preserve image Read references for non-vision models, 8 files) and confirmed the Desktop image-notice composition and the absence of apackages/cliTUI ingestion path. - Checked exact-head CI:
labelSUCCESS,testSUCCESS (run 33162772900),windows_recoverySUCCESS, OPEN/MERGEABLE/BLOCKED/REVIEW_REQUIRED.
Findings from Sol's review (file:line anchored):
Standards — NO-GO — 2×P2 + 1×P3 (worst P2)
-
P2 — Required PR template was replaced —
CONTRIBUTING.md:81-85requires filling.github/pull_request_template.md; the body substitutes customSummary/Desktop evidence/Validation/AI assistancesections and drops the exact AI selector, checklist, and behavior declaration. The body does disclose Codex, includes UI media/validation, and the sole commit hasGenerated-by: Codex, so this is counted as one template-root finding. Restore the template around the existing content. -
P2 — No composed contract oracle for the user-visible notice — renderer rules at
apps/desktop/src/renderer/README.md:78-83require source plus focused contract tests.attachment-input-notice.test.ts:38-63proves only the boolean predicate;new-task-staged-content.test.ts:184-204proves only that an injected callback fires. Removingapp-shell.tsx:407-415, swapping localized copy, or passing stale target data all leave both tests green. Add a focusedstaged-image → selected-model → localized toastApi.info(title, description)test (EN/zh, vision/no-target negatives). -
P3 — Stale Runtime API comment —
packages/runtime/src/ai-sdk-backend.ts:843-846still states that false/unknown vision support keeps refs “with a fallback note,” while the implementation deletes the refs and the tests now assert the absence of that note. Update the stale contract prose.
No remaining Fowler smell was found; entropy rises slightly from the untested composition and stale comment despite simpler prompt logic.
Spec — NO-GO — 1×P1 + 1×P2 (worst P1)
-
P1 — Required TUI attachment-ref path is entirely missing — #4079 explicitly requires TUI
[image N]to preserve itspath/session-resourceref in model-facing content and requires a serialization test. This diff changes nopackages/clisource/test. The sole TUI submit choke point acceptstext/optional modelTextonly atpackages/cli/src/pi-tui-runner.ts:1028-1044;MakaSubmitMessageOptionshas no attachment field atsession-driver.ts:106-112; the Host driver serializes only{text, displayText}atruntime-host-session-driver.ts:507-537. Therefore submitting a[image N]label cannot place anAttachmentRefintoturn.message.submit.content, so Runtime cannot fold aReadargument. Implement the TUI ingestion/serialization path and its regression test. -
P2 — Desktop notice is not one-time — every successful picker and every drag/paste batch invokes the callback at
use-composer-attachments.ts:216-245, andapp-shell.tsx:407-415emitstoastApi.infoon every non-vision image batch with no seen-state. Staging image A then image B in one non-vision task makes the exact notice appear twice. Add scoped dedupe (e.g., per-task/sessionSet) and a two-stage regression proving the second batch is silent.
Validated/excluded on this head: non-vision current content does retain the exact Read ref/path, name and MIME via model-history.ts:929-956; fallback recommendations are removed; native bytes remain gated on supportsVision === true; current/replay/steering and mixed-refs remain intact; no OCR/model-switch/PDF broadening; English notice copy exactly matches #4079.
Verification: git diff --check PASS, targeted Biome 8/8 PASS, ASF headers PASS, merge-base a6a08b81 clean, worktree clean, head unchanged. label/test/windows_recovery all terminal SUCCESS. No reviews/comments were read before seal; no GitHub writes.
What I did not judge: a true TUI [image N] → AttachmentRef → Read E2E and a two-stage Desktop toast dedupe E2E were not executed beyond code inspection — verification was by code inspection and the unit tests noted above.
Gate: Standards 2×P2 + Spec 1×P1 + 1×P2 remain; despite label/test/windows_recovery green, head 780866e is not merge-ready as “preserve image Read references for non-vision models” until the TUI serialization path and the one-time notice guard are closed. Seal: notes/pr-4080-provisional.md.
Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
Summary
Closes #4079.
Readreferences.Desktop evidence
A Chinese Desktop fixture with the explicit non-vision
text-only-v1model keepsnotice.pngstaged and shows the advisory notice.Validation
npm --workspace @maka/desktop test -- --test-name-pattern='shows an advisory notice only for images on a non-vision model|reports newly staged image attachments'npm --workspace @maka/runtime run test:dist -- --test-name-pattern='current-turn image attachment keeps its Read reference|does not read image bytes for a non-vision model'npx tsc -p apps/desktop/tsconfig.renderer.json --noEmitnpx tsc -p apps/desktop/tsconfig.main.json --noEmitnpm --workspace @maka/runtime run buildnpm --workspace @maka/ui run buildnpm --workspace @maka/desktop run build:workspace-depsAI assistance
Generated with Codex; reviewed locally in two independent passes. Human review is still required before merge.