feat(ui): render session attachment images in Markdown - #4137
Conversation
8c8193d to
e3061e7
Compare
e848547 to
2083cc0
Compare
jackwener
left a comment
There was a problem hiding this comment.
I found no P0–P3 issues on exact head 2083cc0ed9fe8e09929af7fc5b295982d7ab8a9f.
The Markdown image URL is the trust boundary, and this change keeps it closed. MarkdownImage is the only chokepoint: canonical maka://runtime/attachments/<artifactId> refs resolve through the existing attachments:readBytes channel against the session that owns the transcript; HTTP(S) keeps the current inline presentation; file:, data:, other maka:// hosts, query strings, and non-canonical ids stay [alt]. The ref never carries a session id, so model text cannot name another session. Assistant previews go through decideImageReadOutcome (raster MIME allowlist, 2MB cap, no SVG). User thumbnails keep their existing payload contract.
Conversation copy rewrites those refs through parseAttachmentResourceRef and the Artifact id map, including Runtime ledger text, and leaves non-canonical spellings untouched.
I did not treat issue #4093 as evidence. The tests can show the claim false: an attachment ref becomes an <img> with a data URL on the current session; unreadable, oversized, and disallowed sources do not; a missing reader stays a placeholder; copy maps source Artifact ids and does not rewrite ?session=.
I am not merging. This is a feature; merge is a human call. Hosted test was still in progress when I posted. This review does not claim CI is green.
Posted by an automated review agent operated by @WAWQAQ. This is not an
independent human review and does not satisfy the committer review required by
CONTRIBUTING.md. A human is accountable for this comment — please push back if
anything here is wrong.
简体中文
精确 head 2083cc0ed9fe8e09929af7fc5b295982d7ab8a9f 上我没有发现 P0–P3。
Markdown 图片 URL 是信任边界,这次改动把它关着。唯一关口是 MarkdownImage:规范的 maka://runtime/attachments/<artifactId> 走现有的 attachments:readBytes,对着当前转录所属的 session 解析;HTTP(S) 保持原来的行内展示;file:、data:、其它 maka:// host、query、非规范 id 都落成 [alt]。ref 不带 session id,模型文本不能点名另一个 session。助手预览走 decideImageReadOutcome(光栅 MIME 白名单、2MB 上限、没有 SVG)。用户缩略图仍用原来的 payload 契约。
会话拷贝用 parseAttachmentResourceRef 和 Artifact id 映射改写这些 ref,包括 Runtime ledger 文本,非规范写法不动。
我没有把 issue #4093 当证据。测试能证伪核心主张:附件 ref 会变成当前 session 上的 data URL 图片;读不到、过大、不允许的源不会;没有 reader 时是占位;拷贝会映射源 Artifact id,且不改写 ?session=。
我不合入。这是功能,合入由人类决定。发这条时 hosted test 还在跑,这次审查不表示 CI 已绿。
本条评论由 @WAWQAQ 运行的自动化审查程序发出。它不构成 CONTRIBUTING.md
所要求的独立人类审查,也不能替代人类审查。有人类对本条评论负责,如有错误请直接指出。
M4n5ter
left a comment
There was a problem hiding this comment.
I found one P1 and one P2 on exact head 2083cc0ed9fe8e09929af7fc5b295982d7ab8a9f. The P1 blocks approval: the 2 MiB assistant-preview guard runs only after the Desktop host has materialized and transferred payloads of up to 50 MiB, while distinct Markdown refs have no load budget. The P2 is a recoverable branch/revision failure: the new ref rewrite is not used by the production copy mode.
The closed URL policy, stale-completion cancellation, identical-ref deduplication, transient-failure retryability, and Quote Companion wiring otherwise held up. I ran 113 focused UI/runtime tests, all workspace dependency builds, all four Desktop TypeScript checks, the Desktop artifact IPC test, and the production renderer build. windows_recovery is green; hosted test was still running when I posted.
Posted by an automated review agent operated by @M4n5ter. This is not an
independent human review and does not satisfy the committer review required by
CONTRIBUTING.md. A human is accountable for this comment — please push back if
anything here is wrong.
简体中文
本条评论由 @M4n5ter 运行的自动化审查程序发出。它不构成 CONTRIBUTING.md
所要求的独立人类审查,也不能替代人类审查。有人类对本条评论负责,如有错误请直接指出。
zhiiw
left a comment
There was a problem hiding this comment.
Reviewed at exact head 2083cc0e (20 files, +586/−168).
No P0–P3 findings. The claims I checked hardest, verified rather than trusted:
- The deleted pre-parse scanner stays deleted safely: the installed Astryx 0.5.0 routes both inline and standalone images through
components.image(confirmed innode_modules/@astryxdesign/core/dist/Markdown/Markdown.js— bothcase 'image'branches honorImageComp, withsanitizeUrlrejectingjavascript:/data:/vbscript:ahead of it), so Maka'sMarkdownImageis genuinely the single policy point.maka:refs pass the sanitizer and resolve through the session attachment reader; everything else still falls back to[alt]. - Attachment loading: concurrent reads deduplicated per session+artifact, the pending entry clears on settle so transient failures stay retryable, and assistant Markdown keeps the raster MIME guard via
safePreview. - Conversation copy: assistant text, ledger text, and user
session_filerefs all rewrite through the artifact id map, so copies resolve their own attachments.
Executed on a real Windows machine at this head: clean rebuild, @maka/ui attachment-image + markdown-body 34/34, runtime conversation-copy/read-model attachment tests green (the 7 other conversation-copy failures on this machine are the pre-existing Windows flake set, identical on the base commit).
简体中文
无 P0–P3。删掉预扫描器是安全的:已在本地安装的 Astryx 0.5.0 里核实内联与独立图片都走 components.image,且 sanitizeUrl 先拦危险 scheme;maka:// 引用经 session 附件读取器解析,其余照旧回落 [alt]。附件加载去重、失败可重试、raster 守卫保留;会话复制的引用改写三处都覆盖了。本机真 Windows 干净重建 + 靶向测试全绿。
xxhZs
left a comment
There was a problem hiding this comment.
Reviewed exact head 2083cc0ed9fe8e09929af7fc5b295982d7ab8a9f. NO-GO: one P1 and one P2, confirmed in the existing inline threads.
The P1 is reachable through ordinary use: up to eight admitted 50 MiB attachments can be named by one assistant answer, and every distinct Markdown ref starts its host read concurrently before the renderer applies the 2 MiB preview cap. The P2 is also on the production path: branch/revision copies use preserve_external, while Markdown refs have no source Session identity, so copied assistant images resolve against the target Session and become placeholders.
The closed URL policy, current-session authority, post-load MIME/size check, pending-read deduplication, retry-on-remount behavior, and Quote Companion wiring otherwise hold. I found no additional P0–P3 issues in the changed surface.
Hosted test and windows_recovery are terminal green, and GitHub reports the exact head mergeable. Those gates do not close the reachable P1.
2083cc0 to
bea6c57
Compare
M4n5ter
left a comment
There was a problem hiding this comment.
I re-reviewed exact head bea6c5715d9c05541fbb8a34a52d6066ab673555 and found no remaining P0–P3 issues.
The previous P1 is fixed. The Desktop host now applies the shared raster MIME and 2 MiB policy to artifact metadata before starting a stream, enforces the same cap while streaming, and returns a typed failure if the stored size drifts. The demonstrated eight-by-50-MiB path therefore starts no byte streams; admitted previews are bounded to 2 MiB each. The renderer still validates the returned MIME and payload before committing it to state, and user thumbnails now use the same policy instead of a separate exception.
I am also retracting my previous P2. I correctly observed that the direct SessionManager copy helper uses preserve_external, but I incorrectly treated that helper as the shipped Desktop branch/revision path. Repository-wide call tracing shows that Desktop goes through sessions:branchFromTurn / sessions:reviseBeforeTurn, DesktopRuntimeHostClient.copySession, and HostSessionRevisionCoordinator. That coordinator copies the artifacts and invokes the existing rewrite with mode: 'exact' plus the copied artifact-id map. The direct helper I probed has no production caller.
I rebuilt Core, Runtime, UI, and Desktop main; ran all four Desktop TypeScript checks; ran 85 focused tests covering the shared policy, Host IPC, Markdown images, materialization, and conversation copying; and completed the production renderer build, formatting check, and diff check. windows_recovery is green on this head. Hosted test and Windows package were still running when I posted, so this approval does not claim those checks are complete.
This remains a feature, so I am not merging it; the product decision remains with a human maintainer.
Posted by an automated review agent operated by @M4n5ter. This is not an
independent human review and does not satisfy the committer review required by
CONTRIBUTING.md. A human is accountable for this comment — please push back if
anything here is wrong.
简体中文
本条评论由 @M4n5ter 运行的自动化审查程序发出。它不构成 CONTRIBUTING.md
所要求的独立人类审查,也不能替代人类审查。有人类对本条评论负责,如有错误请直接指出。
bea6c57 to
eea2d68
Compare
xxhZs
left a comment
There was a problem hiding this comment.
Reviewed exact head eea2d68e3163ddc7ef572598b9e5124acb476e8c. Approved — no P0–P3 findings remain.
The previous P1 is closed at the owning boundary. attachments:readBytes now applies the shared raster policy to metadata before streaming, admits at most 2 MiB, enforces the same cap while consuming chunks, verifies the stored size, and returns typed failures; the renderer independently revalidates MIME and base64 before state. The former eight-by-50-MiB path now starts zero streams. Pending reads remain keyed by Session and Artifact, and the existing Runtime Host request cap bounds transport concurrency.
I also retract my previous P2 after tracing the shipped Desktop path end to end. Desktop branch/revision requests go through DesktopRuntimeHostClient.copySession to HostSessionRevisionCoordinator, which copies conversation Artifacts and calls the rewriter with mode: "exact" plus the Artifact id map. The SessionManager preserve_external helper I cited is not the production Desktop route.
The URL-policy chokepoint, copy rewriting, transient retry behavior, and Quote Companion wiring still hold after the rebase. Fresh merge-tree is clean and git diff --check passes. Hosted checks were still in progress when I submitted this code review.
zhiiw
left a comment
There was a problem hiding this comment.
Re-reviewed at the current head eea2d68e (previous approval was on 2083cc0e).
The branch was rebased and gained one commit. git range-diff shows the five original commits are patch-identical between the two heads; the only new content is eea2d68e "fix: enforce one attachment image preview policy", which this approval covers:
- Single policy authority: the raster-MIME allowlist, extension fallback, and 2 MiB cap moved from
packages/ui/src/artifact-preview-registry.tsinto@maka/core/artifacts(resolveArtifactImagePreview/normalizeArtifactImagePreviewMime). The UI registry now delegates; semantics are unchanged (MIME authoritative when present, extension fallback otherwise, SVG still excluded). - IPC admission tightened to match:
attachments:readBytesinruntime-host-artifacts-ipc-main.tsnow rejects preview-ineligible metadata before streaming (oversize →too_large, SVG/unknown →unsupported_mime), caps the stream itself at the same 2 MiB (mid-stream drift aborts cleanly via a dedicated sentinel, not a generic error), and returns only the normalized allowlisted MIME instead of passingartifact.mimeTypethrough. - The two-policy gap is closed:
useAttachmentImageSourceno longer takessafePreview— the user-thumbnail path (previously unvalidated passthrough ofresult.mimeTypeinto a data URL) and the Markdown path now both go throughdecideImageReadOutcome, andAttachmentImagepre-gates on metadata so oversized/unsupported attachments never trigger a read. I confirmedreadByteshas no non-image consumer: it flows only throughSessionAttachmentProviderto the two image call sites.
Executed on a real Windows machine at this head: clean forced rebuild, desktop artifact-preview-registry + runtime-host-artifacts-ipc-main 6/6 (including the new admission tests: no stream on ineligible metadata, mid-stream oversize abort), @maka/ui attachment-image + markdown-body 35/35.
简体中文
分支 rebase 后多了一个提交。range-diff 确认原有 5 个提交逐字节等价,新内容只有 eea2d68「统一附件图片预览策略」。本 approve 覆盖该增量:图片预览的 MIME 白名单/扩展名回落/2MiB 上限收进 @maka/core 单一权威,UI 侧改为委托;attachments:readBytes 在流式读取前先做准入(超限/非法 MIME 直接拒),流中漂移也能干净中止,且只回规范化后的白名单 MIME;用户缩略图与 Markdown 两条路径合一,都过 decideImageReadOutcome,元数据不合格时不发起读取。已确认 readBytes 没有非图片消费方。本机真 Windows 干净重建:desktop 6/6(含新准入测试)、ui 35/35。
eea2d68 to
c7c1234
Compare
Route every Astryx Markdown image through Maka's component policy, resolve canonical session attachment refs through the existing Runtime Host byte reader, and keep all other schemes inert. Share the session-scoped loader with user attachment thumbnails, remove the old reader prop chain and pre-parse image scanner, and tell the model which attachment ref is a valid Markdown image source. Add streaming, settled, missing-attachment, scheme-policy, model-history, and Storybook coverage. Generated-by: Codex
c7c1234 to
dbc1b60
Compare
Generated-by: Codex
Rewrite canonical attachment resource references when conversations are copied so branched and companion transcripts keep rendering the copied Artifact. Route Quote Companion through the existing Workbar attachment service, reuse the renderer image allowlist and payload cap, and share repeated reads per Session provider. Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
dbc1b60 to
ed4eb7a
Compare
Summary
Render canonical
maka://runtime/attachments/<artifactId>image refs in assistant Markdown without widening the Markdown URL policy or adding another attachment transport.Astryx 0.5.0 already routes inline and standalone images through
components.image(facebook/astryx#4100), so this change makes Maka'sMarkdownImagethe single policy chokepoint and deletes the obsolete pre-parse Markdown scanner. Canonical session refs resolve through the existingattachments:readBytescapability; HTTP(S) images retain their current presentation, and every other scheme falls back to[alt].The session reader now enters
@maka/uionce atChatView. A session-scoped provider and shared loading hook serve both assistant Markdown and user attachment thumbnails, replacing the previousChatView -> TurnView -> UserMessageBodyreader prop chain. The provider only deduplicates concurrent reads, so transient failures remain retryable. One core raster-preview policy now governs every transcript image: the Desktop host rejects ineligible MIME metadata and payloads over 2 MiB before streaming, rechecks the byte cap while streaming, and the renderer validates the returned MIME/base64 again before committing it to state. Quote Companion receives the same capability through its existing Workbar attachment service.Conversation copies rewrite canonical attachment refs in both stored assistant messages and Runtime ledger text through the existing Artifact id map. Canonical user attachment storage refs also follow that map, so branches, revisions, and companion forks resolve copied Artifacts rather than source-Session ids. The model-facing attachment block identifies image refs as valid Markdown image sources.
Before / after
Fixes #4093
Verification
npm run build --workspace @maka/ui && node --test packages/ui/dist/__tests__/attachment-image.test.js packages/ui/dist/__tests__/markdown-body.test.js— 35/35 passednpm run build --workspace @maka/runtime && node --test packages/runtime/dist/__tests__/conversation-copy.test.js packages/runtime/dist/__tests__/runtime-event-read-model.test.js— 79/79 passednpm run build:main --workspace @maka/desktop && node --test apps/desktop/dist/main/__tests__/runtime-host-artifacts-ipc-main.test.js apps/desktop/dist/main/__tests__/artifact-preview-registry.test.js— 6/6 passednpm run build:workspace-deps --workspace @maka/desktop && npm run typecheck --workspace @maka/desktop— passed, including preload, main, renderer, and Storybooknpm run format:check— passedProduct/Attachments/AssistantMarkdownImagethrough the realChatViewcomposition — resolved adata:image/pngsource, computeddisplay: blockandmax-width: 100%, rendered 256px wide inside the 800px assistant content column, with no console warnings or errorsRepository-wide tests were not run.
Review focus
The Markdown ref never carries a session id.
parseAttachmentResourceRefadmits only the canonical Runtime Host attachment form, andSessionAttachmentProvidersupplies the session that owns the rendered transcript. Missing readers, unknown/deleted artifacts, malformed refs, disallowed MIME types, oversized transcript previews,data:,file:, and othermaka://hosts remain inert placeholders. Conversation copy owns ref rewriting when Artifact ids change.AI use
Tool(s) and scope: Codex diagnosed the Astryx seam, implemented the UI/runtime changes and tests, performed the adversarial review, and verified the focused suites and Storybook composition. Commits carry
Generated-by: Codextrailers.Checklist
Does this PR entail a change in behavior?