Skip to content

feat(attachments): preserve image Read references for non-vision models - #4080

Open
me2seeks wants to merge 1 commit into
apache:mainfrom
me2seeks:feat/4079-attachment-delivery
Open

feat(attachments): preserve image Read references for non-vision models#4080
me2seeks wants to merge 1 commit into
apache:mainfrom
me2seeks:feat/4079-attachment-delivery

Conversation

@me2seeks

@me2seeks me2seeks commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Summary

Closes #4079.

  • Keep staged images available to every model through their existing attachment Read references.
  • Add provider-native image content only when the selected model explicitly supports vision.
  • Show a non-blocking Desktop notice when an image is staged for a model without native vision support.

Desktop evidence

A Chinese Desktop fixture with the explicit non-vision text-only-v1 model keeps notice.png staged and shows the advisory notice.

Desktop image attachment notice in Chinese

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 --noEmit
  • npx tsc -p apps/desktop/tsconfig.main.json --noEmit
  • npm --workspace @maka/runtime run build
  • npm --workspace @maka/ui run build
  • npm --workspace @maka/desktop run build:workspace-deps

AI assistance

Generated with Codex; reviewed locally in two independent passes. Human review is still required before merge.

@me2seeks
me2seeks marked this pull request as ready for review August 28, 2026 10:32
@github-actions github-actions Bot added the effort/M Under 500 readable lines label Aug 28, 2026
@Astro-Han

Copy link
Copy Markdown
Contributor

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 a packages/cli TUI ingestion path.
  • Checked exact-head CI: label SUCCESS, test SUCCESS (run 33162772900), windows_recovery SUCCESS, OPEN/MERGEABLE/BLOCKED/REVIEW_REQUIRED.

Findings from Sol's review (file:line anchored):

Standards — NO-GO — 2×P2 + 1×P3 (worst P2)

  1. P2 — Required PR template was replacedCONTRIBUTING.md:81-85 requires filling .github/pull_request_template.md; the body substitutes custom Summary/Desktop evidence/Validation/AI assistance sections and drops the exact AI selector, checklist, and behavior declaration. The body does disclose Codex, includes UI media/validation, and the sole commit has Generated-by: Codex, so this is counted as one template-root finding. Restore the template around the existing content.

  2. P2 — No composed contract oracle for the user-visible notice — renderer rules at apps/desktop/src/renderer/README.md:78-83 require source plus focused contract tests. attachment-input-notice.test.ts:38-63 proves only the boolean predicate; new-task-staged-content.test.ts:184-204 proves only that an injected callback fires. Removing app-shell.tsx:407-415, swapping localized copy, or passing stale target data all leave both tests green. Add a focused staged-image → selected-model → localized toastApi.info(title, description) test (EN/zh, vision/no-target negatives).

  3. P3 — Stale Runtime API commentpackages/runtime/src/ai-sdk-backend.ts:843-846 still 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)

  1. P1 — Required TUI attachment-ref path is entirely missingfeat(attachments): surface direct-image delivery without blocking attachment access #4079 explicitly requires TUI [image N] to preserve its path/session-resource ref in model-facing content and requires a serialization test. This diff changes no packages/cli source/test. The sole TUI submit choke point accepts text/optional modelText only at packages/cli/src/pi-tui-runner.ts:1028-1044; MakaSubmitMessageOptions has no attachment field at session-driver.ts:106-112; the Host driver serializes only {text, displayText} at runtime-host-session-driver.ts:507-537. Therefore submitting a [image N] label cannot place an AttachmentRef into turn.message.submit.content, so Runtime cannot fold a Read argument. Implement the TUI ingestion/serialization path and its regression test.

  2. 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, and app-shell.tsx:407-415 emits toastApi.info on 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/session Set) 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]AttachmentRefRead 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/M Under 500 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(attachments): surface direct-image delivery without blocking attachment access

2 participants