feat(terminal): support image prompt attachments - #249
Merged
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR adds ordered text and image message parts across database storage, assistant execution, provider conversion, context estimation, compaction, and terminal workflows. Terminal prompts can include validated PNG or WebP attachments that persist through sessions and provider requests. ChangesMultipart image message support
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Terminal
participant AssistantRuntime
participant SessionRepository
participant Provider
Terminal->>Terminal: Read and validate clipboard image
Terminal->>AssistantRuntime: Submit prompt draft with images
AssistantRuntime->>AssistantRuntime: Clone and validate request
AssistantRuntime->>SessionRepository: Persist ordered message parts
AssistantRuntime->>Provider: Convert and send multipart messages
Provider-->>AssistantRuntime: Return completion
AssistantRuntime->>Terminal: Restore prompt and attachment state
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #249 +/- ##
==========================================
+ Coverage 84.77% 84.80% +0.02%
==========================================
Files 316 320 +4
Lines 29691 30586 +895
==========================================
+ Hits 25172 25939 +767
- Misses 3095 3174 +79
- Partials 1424 1473 +49
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (6)
internal/terminal/compact_commands_internal_test.go (1)
184-199: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winTest image attachment preservation at queue lifecycle boundaries.
These tests use
promptDraft, but they only exercise text. A regression that removespromptDraft.Imagescan pass all three tests.
internal/terminal/compact_commands_internal_test.go#L184-L199: Queue a draft with an image and assert that the resulting prompt request preserves the complete image attachment.internal/terminal/compact_commands_internal_test.go#L527-L537: Restore an image-bearing queued draft after compaction failure and assert thatapp.composerImagespreserves the attachment.internal/terminal/prompt_cancel_internal_test.go#L26-L39: Keep an image-bearing queued draft during cancellation and assert that its complete attachment remains inapp.queuedMessages.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/terminal/compact_commands_internal_test.go` around lines 184 - 199, Extend the tests at internal/terminal/compact_commands_internal_test.go lines 184-199, internal/terminal/compact_commands_internal_test.go lines 527-537, and internal/terminal/prompt_cancel_internal_test.go lines 26-39 to use image-bearing promptDraft values and assert complete image preservation at each queue lifecycle boundary: in the resulting prompt request, restored app.composerImages after compaction failure, and app.queuedMessages after cancellation.internal/terminal/extension_events_internal_test.go (1)
99-102: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert the complete queued image attachment.
Lines 101-102 only verify the image count and
Data. A queue copy can discardName,MIMEType,Width, orHeightand still pass this test. Compare the completeimageAttachment.Proposed test update
require.Len(t, app.queuedMessages[0].Images, 1) -assert.Equal(t, []byte{1}, app.queuedMessages[0].Images[0].Data) +assert.Equal(t, imageAttachment{ + Name: testImageAttachmentName, MIMEType: clipboardImageMIME, + Data: []byte{1}, Width: 1, Height: 1, +}, app.queuedMessages[0].Images[0])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/terminal/extension_events_internal_test.go` around lines 99 - 102, Update the queued image assertions in the test to compare the entire imageAttachment value, including Name, MIMEType, Width, Height, and Data, rather than separately checking only the count and Data. Preserve the existing queued message and attachment-count assertions.internal/assistant/llm_conversion_internal_test.go (1)
90-120: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a table-driven test for the multipart conversion cases.
These cases exercise the same conversion path. Put them in one table-driven test and share the common assertions.
As per coding guidelines,
**/*_test.go: Prefer table-driven tests for core behavior and regression tests for terminal rendering bugs in Go.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/assistant/llm_conversion_internal_test.go` around lines 90 - 120, Refactor TestLLMMessageFromDatabasePreservesOrderedMultipartImageOnly into a table-driven test covering both the text-plus-image and image-only entities. Define shared expected content and image metadata in each case, run the cases with subtests, and retain the existing conversion, ordering, encoding, and image-only assertions.Source: Coding guidelines
internal/database/session_message_repository.go (1)
183-191: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueDrop the redundant
cloneBytesin the insert argument.
sessionMessageFromEntryat Line 216 already clones every part, includingData, somessage.Parts[sequence].Datais repository-owned at this point. The driver copies the bytes duringExec. The extracloneBytescall allocates a second full copy of each image payload on the write path with no ownership benefit.♻️ Proposed change
- part.Text, part.MIMEType, part.Name, part.Width, part.Height, cloneBytes(part.Data), + part.Text, part.MIMEType, part.Name, part.Width, part.Height, part.Data,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/database/session_message_repository.go` around lines 183 - 191, Remove the redundant cloneBytes call from the part.Data argument in the append-message-part transaction within the message part insertion loop. Pass the repository-owned Data directly to transaction.Exec while leaving the existing sessionMessageFromEntry cloning behavior and all other insert arguments unchanged.internal/assistant/runtime_model.go (1)
121-126: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe
contextHasImagescapability check duplicates the check at Line 151.Line 151 validates
build.Request.Messagesfor image support after request preparation. Those messages come from the same conversation context thatcontextHasImagesdescribes. The pre-authentication check therefore adds a second gate for the same condition. Keeping only the post-preparation check would letcontextHasImagesbe dropped frommodelResponseentirely, which also removes the parameter added at Line 108.One behavioral difference exists: the early check fails before authentication and before compaction runs. If failing earlier is intentional, add a short comment that states why.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/assistant/runtime_model.go` around lines 121 - 126, Remove the redundant pre-authentication image capability check guarded by contextHasImages in modelResponse, keeping the post-preparation validation of build.Request.Messages as the single gate. Then remove contextHasImages from modelResponse’s parameters and any now-unused setup or references; if the early validation is intentionally retained, document why it must occur before authentication and compaction instead.internal/assistant/lifecyclepayload/lifecyclepayload_test.go (1)
35-49: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd attachment payload cases.
Add table cases with nonempty attachments. Assert
attachments,attachment_count, and the absence of raw image data. The current nil-only case does not verify the new lifecycle contract.As per coding guidelines,
**/*_test.go: Prefer table-driven tests for core behavior and regression tests for terminal rendering bugs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/assistant/lifecyclepayload/lifecyclepayload_test.go` around lines 35 - 49, Extend the lifecycle payload test around lifecyclepayload.Prompt with table-driven cases covering nonempty attachments in addition to the existing nil case. For attachment cases, assert the attachments and attachment_count fields and verify raw image data is absent, while preserving the current assertions for prompt metadata and nil attachments.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/assistant/llm_conversion_internal_test.go`:
- Around line 104-120: Strengthen the assertions in the test around
llmMessageFromDatabase by verifying the mixed message’s text content value in
addition to its type. For the imageOnly result, assert that the sole part has
type llm.PartImage and data equal to the Base64-encoded image bytes, while
retaining the existing conversion and length checks.
In `@internal/assistant/prompt_images.go`:
- Around line 165-175: Add WebP decoding support in imageMIMEType by importing
golang.org/x/image/webp and registering the "webp" format as "image/webp", with
validation coverage for direct PromptRequest images. Update
internal/assistant/prompt_images.go at lines 165-175;
internal/provider/image_content.go lines 50-57 requires no direct change—retain
its existing WebP validation.
In `@internal/assistant/runtime_model.go`:
- Around line 48-49: Update the slash-command dispatch in the runtime response
flow to pass strings.TrimSpace(prompt) to respondToSlashCommand instead of the
original prompt, ensuring leading or trailing whitespace is removed before
splitSlashCommand processes the command.
- Around line 62-75: Update the image-detection logic around
promptContextContainsImages so cache lookup does not build or traverse the full
active lineage. Replace it with a cheap session_message_parts existence check,
or reuse context already produced by prepareCompletionRequestWithAutoCompaction,
while preserving image-bearing prompts’ durable multipart execution behavior and
avoiding duplicate context construction on cache misses.
In `@internal/database/session_entry_repository.go`:
- Line 55: Update the entry-reading methods Entries, Children, Entry, and
LeafEntry so each hydrates Message.Parts when constructing Message values,
preserving image attachments in all returned results; ensure Tree receives the
same behavior through its Entries path.
In `@internal/database/validation.go`:
- Around line 186-191: Update validImageMIMEType to reject wildcard image
subtypes such as "image/*" while continuing to accept concrete, lowercase image
media types. Add the wildcard check alongside the existing prefix and length
validation before the value is considered valid.
In `@internal/provider/anthropic.go`:
- Around line 458-467: The empty-content guards in anthropic.go (458-467) and
openai_chat.go (210-219) only detect empty string values, not empty structured
user-content lists. Update the checks in the flows using anthropicUserContent
and openAIChatUserContent to skip both empty text and empty structured content
before appending messages.
In `@internal/terminal/agent_tasks.go`:
- Around line 1709-1721: Update the message construction in
appendMissingSessionMessages to populate Attachments with
databaseAttachmentSummaries(message.Parts) instead of nil, while preserving
imageAttachmentsFromDatabase(message.Parts) for promptDraft history.
In `@internal/terminal/session_view.go`:
- Around line 89-97: Update the clone branch in the session view copy logic to
deep-copy promptHistory alongside promptHistoryImages, and make the same change
in the restoration logic around the existing line-152 path. Ensure
recordPromptDraftHistory mutates only the copied history so saved and restored
views retain matching text and image entries.
---
Nitpick comments:
In `@internal/assistant/lifecyclepayload/lifecyclepayload_test.go`:
- Around line 35-49: Extend the lifecycle payload test around
lifecyclepayload.Prompt with table-driven cases covering nonempty attachments in
addition to the existing nil case. For attachment cases, assert the attachments
and attachment_count fields and verify raw image data is absent, while
preserving the current assertions for prompt metadata and nil attachments.
In `@internal/assistant/llm_conversion_internal_test.go`:
- Around line 90-120: Refactor
TestLLMMessageFromDatabasePreservesOrderedMultipartImageOnly into a table-driven
test covering both the text-plus-image and image-only entities. Define shared
expected content and image metadata in each case, run the cases with subtests,
and retain the existing conversion, ordering, encoding, and image-only
assertions.
In `@internal/assistant/runtime_model.go`:
- Around line 121-126: Remove the redundant pre-authentication image capability
check guarded by contextHasImages in modelResponse, keeping the post-preparation
validation of build.Request.Messages as the single gate. Then remove
contextHasImages from modelResponse’s parameters and any now-unused setup or
references; if the early validation is intentionally retained, document why it
must occur before authentication and compaction instead.
In `@internal/database/session_message_repository.go`:
- Around line 183-191: Remove the redundant cloneBytes call from the part.Data
argument in the append-message-part transaction within the message part
insertion loop. Pass the repository-owned Data directly to transaction.Exec
while leaving the existing sessionMessageFromEntry cloning behavior and all
other insert arguments unchanged.
In `@internal/terminal/compact_commands_internal_test.go`:
- Around line 184-199: Extend the tests at
internal/terminal/compact_commands_internal_test.go lines 184-199,
internal/terminal/compact_commands_internal_test.go lines 527-537, and
internal/terminal/prompt_cancel_internal_test.go lines 26-39 to use
image-bearing promptDraft values and assert complete image preservation at each
queue lifecycle boundary: in the resulting prompt request, restored
app.composerImages after compaction failure, and app.queuedMessages after
cancellation.
In `@internal/terminal/extension_events_internal_test.go`:
- Around line 99-102: Update the queued image assertions in the test to compare
the entire imageAttachment value, including Name, MIMEType, Width, Height, and
Data, rather than separately checking only the count and Data. Preserve the
existing queued message and attachment-count assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 346f1780-1bf4-4da9-9006-52468c9b28f5
📒 Files selected for processing (90)
cmd/librecode/cli_helpers_internal_test.gocmd/librecode/prompt.gointernal/agenttask/runtime_runner.gointernal/assistant/context_compaction_test.gointernal/assistant/lifecycle.gointernal/assistant/lifecyclepayload/behavior_test.gointernal/assistant/lifecyclepayload/lifecyclepayload.gointernal/assistant/lifecyclepayload/lifecyclepayload_test.gointernal/assistant/llm_conversion.gointernal/assistant/llm_conversion_internal_test.gointernal/assistant/prompt_images.gointernal/assistant/prompt_images_internal_test.gointernal/assistant/runtime.gointernal/assistant/runtime_context_internal_test.gointernal/assistant/runtime_entries.gointernal/assistant/runtime_model.gointernal/assistant/runtime_persist.gointernal/assistant/runtime_test.gointernal/assistant/test_message_helpers_internal_test.gointernal/compaction/plan.gointernal/compaction/plan_internal_test.gointernal/contextwindow/tokens.gointernal/contextwindow/usage_internal_test.gointernal/contextwindow/usage_led_internal_test.gointernal/database/entity.gointernal/database/migrations/00013_add_session_message_parts.sqlinternal/database/migrations_test.gointernal/database/repository_helpers_internal_test.gointernal/database/session_compaction_input_internal_test.gointernal/database/session_entry_repository.gointernal/database/session_message_parts_test.gointernal/database/session_message_repository.gointernal/database/session_repository_test.gointernal/database/session_store.gointernal/database/session_usage_test.gointernal/database/sqlite_contention_internal_test.gointernal/database/task_validation_internal_test.gointernal/database/test_helpers_test.gointernal/database/validation.gointernal/extension/lua_values.gointernal/model/message_filter.gointernal/model/messages.gointernal/model/messages_test.gointernal/provider/anthropic.gointernal/provider/anthropic_mapping_internal_test.gointernal/provider/image_content.gointernal/provider/image_content_internal_test.gointernal/provider/messages.gointernal/provider/messages_internal_test.gointernal/provider/openai_chat.gointernal/provider/openai_chat_payload_internal_test.gointernal/provider/openai_responses.gointernal/terminal/agent_tasks.gointernal/terminal/agent_tasks_behavior_internal_test.gointernal/terminal/agent_tasks_live_internal_test.gointernal/terminal/app.gointernal/terminal/async_events_internal_test.gointernal/terminal/attachment_actions.gointernal/terminal/attachments.gointernal/terminal/attachments_internal_test.gointernal/terminal/auth_commands_internal_test.gointernal/terminal/clipboard.gointernal/terminal/clipboard_internal_test.gointernal/terminal/compact_commands_internal_test.gointernal/terminal/extension_events_internal_test.gointernal/terminal/input.gointernal/terminal/input_escape.gointernal/terminal/interrupt_internal_test.gointernal/terminal/keybindings.gointernal/terminal/message_render.gointernal/terminal/model_test_helpers_internal_test.gointernal/terminal/panel_session_selection_internal_test.gointernal/terminal/panel_test_helpers_internal_test.gointernal/terminal/panel_tree_internal_test.gointernal/terminal/prompt_cancel_internal_test.gointernal/terminal/prompt_history.gointernal/terminal/prompt_history_internal_test.gointernal/terminal/prompt_queue.gointernal/terminal/prompt_queue_internal_test.gointernal/terminal/prompt_response_internal_test.gointernal/terminal/prompt_send.gointernal/terminal/prompt_send_internal_test.gointernal/terminal/prompt_submit.gointernal/terminal/render_composer.gointernal/terminal/render_internal_test.gointernal/terminal/render_parity_internal_test.gointernal/terminal/running_tools_internal_test.gointernal/terminal/session_commands_internal_test.gointernal/terminal/session_view.gointernal/terminal/workflow_summary_internal_test.go
omarluq
force-pushed
the
feat/image-attachments
branch
from
August 4, 2026 04:34
1edea2a to
5fbff3d
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
internal/terminal/compact_commands_internal_test.go (1)
202-210: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the ordered text part.
Line 203 checks legacy
MessageEntity.Content. It does not checkmessage.Parts[0]. A regression can retainContentbut create an empty or invalid first multipart text part. Assert thatmessage.Parts[0]is the canonical text part and contains"queued after compact"before checking the image part.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/terminal/compact_commands_internal_test.go` around lines 202 - 210, The test should validate the canonical ordered text part, not only legacy MessageEntity.Content. In the assertions for the final message, add checks that message.Parts[0] is the text part and contains "queued after compact" before the existing message.Parts[1] image assertions.internal/database/session_message_parts_test.go (1)
174-203: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a table-driven test for validation cases.
The image-count, MIME-type, and pixel-limit cases repeat the same append-and-error flow. Define cases with
Partsand the expected error text, then execute them witht.Run.As per coding guidelines, "
**/*_test.go: Prefer table-driven tests for core behavior and regression tests for terminal rendering bugs in Go."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/database/session_message_parts_test.go` around lines 174 - 203, Refactor the repeated validation checks in the message-parts test around repository.AppendMessage into a table-driven test. Define cases containing the invalid Parts value and expected error text, then iterate with t.Run, preserving the existing image-count, MIME-type, and pixel-limit assertions.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/database/migrations/00013_add_session_message_parts.sql`:
- Around line 2-3: Update the migration’s schema creation statements for the
migration-defined index and table to avoid adopting pre-existing objects with
the same names; use strict creation semantics and ensure the Down rollback only
removes objects created by this migration rather than dropping pre-existing
schema objects.
- Line 9: Update the sequence column constraint in the migration to require
SQLite integer storage and non-negative values by checking typeof(sequence) =
'integer' alongside sequence >= 0. Preserve the existing NOT NULL requirement.
- Line 6: Update the session_message_parts table definition so the id column is
explicitly declared NOT NULL alongside its TEXT PRIMARY KEY constraint,
preventing records without an ID.
---
Nitpick comments:
In `@internal/database/session_message_parts_test.go`:
- Around line 174-203: Refactor the repeated validation checks in the
message-parts test around repository.AppendMessage into a table-driven test.
Define cases containing the invalid Parts value and expected error text, then
iterate with t.Run, preserving the existing image-count, MIME-type, and
pixel-limit assertions.
In `@internal/terminal/compact_commands_internal_test.go`:
- Around line 202-210: The test should validate the canonical ordered text part,
not only legacy MessageEntity.Content. In the assertions for the final message,
add checks that message.Parts[0] is the text part and contains "queued after
compact" before the existing message.Parts[1] image assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: cceb90e3-250a-46f6-b60c-3e3fb9bf8fe3
📒 Files selected for processing (97)
cmd/librecode/cli_helpers_internal_test.gocmd/librecode/prompt.gogo.modinternal/agenttask/runtime_runner.gointernal/assistant/context_compaction_test.gointernal/assistant/lifecycle.gointernal/assistant/lifecyclepayload/behavior_test.gointernal/assistant/lifecyclepayload/lifecyclepayload.gointernal/assistant/lifecyclepayload/lifecyclepayload_test.gointernal/assistant/llm_conversion.gointernal/assistant/llm_conversion_internal_test.gointernal/assistant/prompt_images.gointernal/assistant/prompt_images_internal_test.gointernal/assistant/runtime.gointernal/assistant/runtime_context_internal_test.gointernal/assistant/runtime_entries.gointernal/assistant/runtime_model.gointernal/assistant/runtime_persist.gointernal/assistant/runtime_slash.gointernal/assistant/runtime_slash_internal_test.gointernal/assistant/runtime_test.gointernal/assistant/test_constants_internal_test.gointernal/assistant/test_message_helpers_internal_test.gointernal/assistant/testdata/prompt.webpinternal/compaction/plan.gointernal/compaction/plan_internal_test.gointernal/contextwindow/tokens.gointernal/contextwindow/usage_internal_test.gointernal/contextwindow/usage_led_internal_test.gointernal/database/entity.gointernal/database/migrations/00013_add_session_message_parts.sqlinternal/database/migrations/00014_index_image_message_parts.sqlinternal/database/migrations_test.gointernal/database/repository_helpers_internal_test.gointernal/database/session_compaction_input_internal_test.gointernal/database/session_entry_repository.gointernal/database/session_message_parts_test.gointernal/database/session_message_repository.gointernal/database/session_repository_test.gointernal/database/session_store.gointernal/database/session_usage_test.gointernal/database/sqlite_contention_internal_test.gointernal/database/task_validation_internal_test.gointernal/database/test_helpers_test.gointernal/database/validation.gointernal/extension/lua_values.gointernal/model/message_filter.gointernal/model/messages.gointernal/model/messages_test.gointernal/provider/anthropic.gointernal/provider/anthropic_mapping_internal_test.gointernal/provider/image_content.gointernal/provider/image_content_internal_test.gointernal/provider/messages.gointernal/provider/messages_internal_test.gointernal/provider/openai_chat.gointernal/provider/openai_chat_payload_internal_test.gointernal/provider/openai_responses.gointernal/terminal/agent_tasks.gointernal/terminal/agent_tasks_behavior_internal_test.gointernal/terminal/agent_tasks_live_internal_test.gointernal/terminal/app.gointernal/terminal/async_events_internal_test.gointernal/terminal/attachment_actions.gointernal/terminal/attachments.gointernal/terminal/attachments_internal_test.gointernal/terminal/auth_commands_internal_test.gointernal/terminal/clipboard.gointernal/terminal/clipboard_internal_test.gointernal/terminal/compact_commands_internal_test.gointernal/terminal/extension_events_internal_test.gointernal/terminal/input.gointernal/terminal/input_escape.gointernal/terminal/interrupt_internal_test.gointernal/terminal/keybindings.gointernal/terminal/message_render.gointernal/terminal/model_test_helpers_internal_test.gointernal/terminal/panel_session_selection_internal_test.gointernal/terminal/panel_test_helpers_internal_test.gointernal/terminal/panel_tree_internal_test.gointernal/terminal/prompt_cancel_internal_test.gointernal/terminal/prompt_history.gointernal/terminal/prompt_history_internal_test.gointernal/terminal/prompt_queue.gointernal/terminal/prompt_queue_internal_test.gointernal/terminal/prompt_response_internal_test.gointernal/terminal/prompt_send.gointernal/terminal/prompt_send_internal_test.gointernal/terminal/prompt_submit.gointernal/terminal/render_composer.gointernal/terminal/render_internal_test.gointernal/terminal/render_parity_internal_test.gointernal/terminal/running_tools_internal_test.gointernal/terminal/session_commands_internal_test.gointernal/terminal/session_view.gointernal/terminal/session_view_internal_test.gointernal/terminal/workflow_summary_internal_test.go
🚧 Files skipped from review as they are similar to previous changes (79)
- internal/terminal/session_commands_internal_test.go
- internal/agenttask/runtime_runner.go
- internal/terminal/keybindings.go
- internal/assistant/runtime_context_internal_test.go
- internal/database/repository_helpers_internal_test.go
- internal/terminal/prompt_history_internal_test.go
- internal/assistant/lifecycle.go
- internal/assistant/lifecyclepayload/behavior_test.go
- internal/contextwindow/usage_internal_test.go
- internal/assistant/test_message_helpers_internal_test.go
- internal/terminal/model_test_helpers_internal_test.go
- internal/terminal/clipboard_internal_test.go
- internal/extension/lua_values.go
- internal/terminal/render_parity_internal_test.go
- internal/terminal/agent_tasks_live_internal_test.go
- internal/database/test_helpers_test.go
- internal/terminal/async_events_internal_test.go
- internal/assistant/context_compaction_test.go
- internal/terminal/interrupt_internal_test.go
- internal/model/message_filter.go
- cmd/librecode/cli_helpers_internal_test.go
- internal/terminal/auth_commands_internal_test.go
- internal/database/session_compaction_input_internal_test.go
- internal/terminal/attachment_actions.go
- internal/terminal/prompt_send.go
- internal/provider/openai_responses.go
- internal/provider/anthropic.go
- internal/database/task_validation_internal_test.go
- internal/terminal/prompt_submit.go
- internal/terminal/clipboard.go
- internal/compaction/plan.go
- internal/terminal/extension_events_internal_test.go
- internal/terminal/panel_test_helpers_internal_test.go
- internal/terminal/running_tools_internal_test.go
- internal/terminal/panel_tree_internal_test.go
- internal/contextwindow/usage_led_internal_test.go
- internal/assistant/runtime.go
- internal/terminal/attachments.go
- internal/compaction/plan_internal_test.go
- internal/terminal/input_escape.go
- internal/terminal/render_composer.go
- internal/terminal/prompt_history.go
- internal/terminal/prompt_send_internal_test.go
- internal/terminal/message_render.go
- internal/database/sqlite_contention_internal_test.go
- internal/terminal/render_internal_test.go
- cmd/librecode/prompt.go
- internal/database/session_store.go
- internal/database/validation.go
- internal/database/session_usage_test.go
- internal/assistant/lifecyclepayload/lifecyclepayload.go
- internal/terminal/input.go
- internal/terminal/agent_tasks.go
- internal/provider/openai_chat.go
- internal/provider/messages_internal_test.go
- internal/database/session_repository_test.go
- internal/assistant/runtime_model.go
- internal/terminal/prompt_response_internal_test.go
- internal/assistant/llm_conversion_internal_test.go
- internal/terminal/prompt_queue.go
- internal/database/migrations_test.go
- internal/database/entity.go
- internal/provider/image_content.go
- internal/assistant/runtime_entries.go
- internal/terminal/workflow_summary_internal_test.go
- internal/terminal/panel_session_selection_internal_test.go
- internal/provider/messages.go
- internal/assistant/runtime_persist.go
- internal/provider/anthropic_mapping_internal_test.go
- internal/contextwindow/tokens.go
- internal/provider/openai_chat_payload_internal_test.go
- internal/terminal/prompt_queue_internal_test.go
- internal/model/messages_test.go
- internal/assistant/llm_conversion.go
- internal/terminal/session_view.go
- internal/terminal/attachments_internal_test.go
- internal/assistant/runtime_test.go
- internal/model/messages.go
- internal/terminal/app.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/database/session_message_parts_test.go (1)
409-412: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert all persisted image fields.
assertMultipartPartsdoes not verify imageMIMEType,Width,Height, or the second imageName. A hydration regression that clears these fields will pass this test. Compare each image with its completedatabase.MessagePartEntityvalue.Proposed test update
- assert.Equal(t, database.MessagePartImage, parts[1].Type) - assert.Equal(t, []byte{1, 2, 3}, parts[1].Data) - assert.Equal(t, "first.png", parts[1].Name) - assert.Equal(t, database.MessagePartImage, parts[2].Type) - assert.Equal(t, []byte{4, 5}, parts[2].Data) + assert.Equal(t, database.MessagePartEntity{ + Data: []byte{1, 2, 3}, MIMEType: testImageMIME, Name: "first.png", + Type: database.MessagePartImage, Width: 10, Height: 20, + }, parts[1]) + assert.Equal(t, database.MessagePartEntity{ + Data: []byte{4, 5}, MIMEType: "image/jpeg", Name: "second.jpg", + Type: database.MessagePartImage, Width: 30, Height: 40, + }, parts[2])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/database/session_message_parts_test.go` around lines 409 - 412, Update assertMultipartParts to validate every persisted image field: MIMEType, Width, Height, and the second image’s Name. Compare each image part against its complete database.MessagePartEntity value so hydration regressions clearing any field fail the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/database/session_message_parts_test.go`:
- Around line 409-412: Update assertMultipartParts to validate every persisted
image field: MIMEType, Width, Height, and the second image’s Name. Compare each
image part against its complete database.MessagePartEntity value so hydration
regressions clearing any field fail the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 252fedf3-f6cc-42d8-8ab0-b561976ec74f
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (10)
go.modinternal/assistant/runtime_model.gointernal/assistant/runtime_persist.gointernal/database/migrations/00013_add_session_message_parts.sqlinternal/database/migrations/00014_index_image_message_parts.sqlinternal/database/migrations_test.gointernal/database/session_entry_repository.gointernal/database/session_message_parts_test.gointernal/provider/image_content.gointernal/terminal/compact_commands_internal_test.go
🚧 Files skipped from review as they are similar to previous changes (6)
- go.mod
- internal/provider/image_content.go
- internal/database/session_entry_repository.go
- internal/assistant/runtime_model.go
- internal/assistant/runtime_persist.go
- internal/terminal/compact_commands_internal_test.go
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Validation
The first CI attempt hit the known flaky workflow cancellation test; the retry passed.