feat: support adding attachments in ramalama run and ramalama chat - #2627
feat: support adding attachments in ramalama run and ramalama chat#2627Christopher-Chianelli wants to merge 3 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds file-attachment support: new repeatable Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as CLI/ArgParser
participant Shell as RamaLamaShell
participant History as ConversationHistory
participant Provider as OpenAI Provider
participant API as OpenAI API
User->>CLI: ramalama chat --attach file.txt
CLI->>Shell: args.attachments = [PathStr('file.txt')]
User->>Shell: /attach file.jpg
Shell->>Shell: _add_attachment(file.jpg) (isfile & readable)
Shell->>Shell: self.attachments.append(file.jpg)
User->>Shell: prompt about attachments
Shell->>History: append user prompt
Shell->>Shell: for each attachment: OpanAIChatAPIMessageBuilder.load(...)
Shell->>History: append attachment message(s)
Shell->>Shell: self.attachments.clear()
Shell->>Provider: build_payload(conversation_history)
Provider->>Provider: _handle_attachments() -> serialize parts
Provider->>API: POST /chat/completions with mixed content (text + attachments)
API-->>Provider: response
Provider-->>Shell: assistant message
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
|
Fixes #2623 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the ability to attach files to chat and run commands via a new --attach CLI flag and a /attach command within the interactive chat session. The implementation includes updates to the OpenAI provider to support multi-part messages, documentation updates, and new E2E and unit tests. My feedback focuses on removing a redundant local import of perror, optimizing the OpanAIChatAPIMessageBuilder by instantiating it once in the constructor, and improving the robustness of file path parsing for the /attach command.
| # Reload perror here so we can mock it in tests | ||
| from ramalama.common import perror |
There was a problem hiding this comment.
This local import is redundant because perror is already imported at the module level (line 33). Re-importing it inside __init__ to facilitate mocking is considered a bad practice. Instead, tests should patch ramalama.chat.perror directly. Furthermore, this local import only affects __init__, leaving other methods using the module-level reference, which leads to inconsistent behavior.
| self.initialize_mcp() | ||
|
|
||
| self.content: list[str] = [] | ||
| self.attachments: list[PathStr] = [] |
There was a problem hiding this comment.
Consider initializing the message builder once here and reusing it throughout the class instance, rather than instantiating it multiple times in different methods.
self.attachments: list[PathStr] = []
self.builder = OpanAIChatAPIMessageBuilder()| # Handle attachments | ||
| if cmd.startswith("/attach "): | ||
| # capitalization matters for files, so use user_content | ||
| file = user_content.strip()[8:] |
There was a problem hiding this comment.
The extracted file path should be stripped to handle cases where multiple spaces are used between the command and the filename (e.g., /attach file.txt).
| file = user_content.strip()[8:] | |
| file = user_content.strip()[8:].strip() |
There was a problem hiding this comment.
Spaces are valid in the start of and end of files names.
There was a problem hiding this comment.
would the filename be in quotes if that were the case?
There was a problem hiding this comment.
Currently, no. Although that begs the question about if a custom parser should be written and what rules should be followed (ex: should multiple files be allowed to be attached in a single /attach subcommand? Should it follow bash/shell rules or something else? How to escape characters?). Attaching files from the command line doesn't have this problem since bash is the one handling the input, but for the /attach subcommand, we are inside an interactive session and are handling the raw input ourselves.
| builder = OpanAIChatAPIMessageBuilder() | ||
| for attachment in self.attachments: | ||
| self.conversation_history.extend(builder.load(attachment)) |
There was a problem hiding this comment.
Use the builder instance initialized in __init__ instead of creating a new one for every request.
| builder = OpanAIChatAPIMessageBuilder() | |
| for attachment in self.attachments: | |
| self.conversation_history.extend(builder.load(attachment)) | |
| for attachment in self.attachments: | |
| self.conversation_history.extend(self.builder.load(attachment)) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
test/unit/test_file_loader_integration.py (3)
45-47: Unused mock parametermock_request.The
@patch('urllib.request.Request')decorator creates themock_requestparameter, but it's never used in this test. Either remove the patch if it's unnecessary, or document why it's needed (e.g., to prevent actual request construction).🔧 Proposed fix: remove unused patch
`@patch`('urllib.request.urlopen') - `@patch`('urllib.request.Request') - def test_chat_with_file_input_single_file_attachment(self, mock_request, mock_urlopen): + def test_chat_with_file_input_single_file_attachment(self, mock_urlopen):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/test_file_loader_integration.py` around lines 45 - 47, The test function test_chat_with_file_input_single_file_attachment currently receives an unused mock parameter mock_request from the `@patch`('urllib.request.Request') decorator; remove that unnecessary patch decorator (or replace it with a dummy/ignored param if you must keep it) so the test signature no longer includes mock_request, and ensure only `@patch`('urllib.request.urlopen') remains (or, if preventing real Request construction is required, rename the parameter to _mock_request and document why it is kept).
397-400: Unused mock parameter and incorrect docstring.Same issues as above:
mock_requestis unused- The docstring says "Test chat functionality with a single file input" but should describe image attachment testing
🔧 Proposed fix
`@patch`('urllib.request.urlopen') - `@patch`('urllib.request.Request') - def test_chat_with_file_input_single_file_attachment(self, mock_request, mock_urlopen): - """Test chat functionality with a single file input.""" + def test_chat_with_image_input_single_file_attachment(self, mock_urlopen): + """Test chat functionality with a single image file attachment."""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/test_file_loader_integration.py` around lines 397 - 400, The test function test_chat_with_file_input_single_file_attachment has an unused mock parameter mock_request and an incorrect docstring; remove the `@patch`('urllib.request.Request') decorator and the mock_request parameter from the function signature (leaving only mock_urlopen), and update the function docstring to accurately describe that this is testing chat behavior with a single image/file attachment rather than a generic "single file input".
94-98: Unused mock parameter and incorrect docstring.Two issues here:
mock_requestis unused (same as above)- The docstring says "Test chat functionality with a single file input" but this test is for directory upload error handling
🔧 Proposed fix
`@patch`('urllib.request.urlopen') - `@patch`('urllib.request.Request') `@patch`('ramalama.common.perror') - def test_chat_with_directory_upload(self, mock_err, mock_request, mock_urlopen): - """Test chat functionality with a single file input.""" + def test_chat_with_directory_upload(self, mock_err, mock_urlopen): + """Test chat gracefully handles directory attachments with an error message."""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/test_file_loader_integration.py` around lines 94 - 98, The test_chat_with_directory_upload test has an unused patched parameter and a misleading docstring; either remove the unused patch for urllib.request.Request or use the mock_request parameter in the test body, and update the docstring to describe directory upload error handling. Specifically, adjust the `@patch` decorators or the function signature so mock_request is not injected if unused (or reference mock_request in assertions), and change the docstring to something like "Test directory upload error handling" to match the test's purpose (referencing test_chat_with_directory_upload, mock_request, mock_urlopen, mock_err).ramalama/chat.py (1)
512-518: Inconsistent error output:print()vsperror().Line 517 uses
print()for the error message, but line 183 usesperror()for a similar validation error. Usingperror()consistently would ensure error messages go to stderr.🔧 Proposed fix
# Handle attachments if cmd.startswith("/attach "): # capitalization matters for files, so use user_content file = user_content.strip()[8:] if not self._add_attachment(file): - print(f'The file {file} does not exist or cannot be accessed.') + perror(f'The file {file} does not exist or cannot be accessed.') return False🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ramalama/chat.py` around lines 512 - 518, The attachment handler uses print(...) for error output which is inconsistent with the rest of the module; replace the print call in the "/attach " branch so that when self._add_attachment(file) returns False you call perror(...) (the same error helper used elsewhere) with the same or matching message (e.g., "The file {file} does not exist or cannot be accessed.") to send the error to stderr and keep logging consistent; update the handler around cmd.startswith("/attach ") and the file variable passed to self._add_attachment accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/ramalama-chat.1.md`:
- Around line 26-27: Update the second sentence in the docs entry so the
fragment becomes a complete sentence: replace "Can be specified multiple times
to attach multiple files." with "It can be specified multiple times to attach
multiple files." so the two-line description reads as two grammatically complete
sentences.
In `@docs/ramalama-run.1.md`:
- Around line 39-40: Replace the fragment "Can be specified multiple times to
attach multiple files." with a complete sentence for the option description that
follows "Attaches the given file to the initial request.", e.g. "It can be
specified multiple times to attach multiple files." Update the option
description text where the option's help string appears (the line containing
"Can be specified multiple times to attach multiple files.") so the two
sentences read as complete, grammatically correct sentences.
In `@ramalama/chat_providers/openai.py`:
- Around line 55-59: The current construction of content always prepends a text
block ({"type":"text","text":...}) even when message.text is empty; update the
logic in the block that builds content (the content variable and the branch that
calls _handle_attachments for message.attachments) so that you only add the text
part when message.text is truthy, otherwise start content as the attachments
list from _handle_attachments(message.attachments); apply the same change to the
analogous branch that handles content building elsewhere in this file so that
empty text parts are never included in the final returned dict
{**message.metadata, 'content': content, 'role': message.role}.
In `@test/e2e/test_run.py`:
- Around line 300-303: The test currently reopens a tempfile while it's still
open (tempfile.NamedTemporaryFile as temp_file then open(temp_file.name, "w")),
which fails on Windows; change the creation to use
tempfile.NamedTemporaryFile(suffix=".txt", delete=False) so the file can be
reopened, close temp_file before reopening (or create/write directly to
temp_file.name after closing), and ensure you explicitly remove the file
afterward (os.unlink(temp_file.name)) to avoid leaving temp artifacts; update
references to temp_file.name and run_cmd accordingly.
---
Nitpick comments:
In `@ramalama/chat.py`:
- Around line 512-518: The attachment handler uses print(...) for error output
which is inconsistent with the rest of the module; replace the print call in the
"/attach " branch so that when self._add_attachment(file) returns False you call
perror(...) (the same error helper used elsewhere) with the same or matching
message (e.g., "The file {file} does not exist or cannot be accessed.") to send
the error to stderr and keep logging consistent; update the handler around
cmd.startswith("/attach ") and the file variable passed to self._add_attachment
accordingly.
In `@test/unit/test_file_loader_integration.py`:
- Around line 45-47: The test function
test_chat_with_file_input_single_file_attachment currently receives an unused
mock parameter mock_request from the `@patch`('urllib.request.Request') decorator;
remove that unnecessary patch decorator (or replace it with a dummy/ignored
param if you must keep it) so the test signature no longer includes
mock_request, and ensure only `@patch`('urllib.request.urlopen') remains (or, if
preventing real Request construction is required, rename the parameter to
_mock_request and document why it is kept).
- Around line 397-400: The test function
test_chat_with_file_input_single_file_attachment has an unused mock parameter
mock_request and an incorrect docstring; remove the
`@patch`('urllib.request.Request') decorator and the mock_request parameter from
the function signature (leaving only mock_urlopen), and update the function
docstring to accurately describe that this is testing chat behavior with a
single image/file attachment rather than a generic "single file input".
- Around line 94-98: The test_chat_with_directory_upload test has an unused
patched parameter and a misleading docstring; either remove the unused patch for
urllib.request.Request or use the mock_request parameter in the test body, and
update the docstring to describe directory upload error handling. Specifically,
adjust the `@patch` decorators or the function signature so mock_request is not
injected if unused (or reference mock_request in assertions), and change the
docstring to something like "Test directory upload error handling" to match the
test's purpose (referencing test_chat_with_directory_upload, mock_request,
mock_urlopen, mock_err).
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5599ecca-4d95-426d-a068-868d73cd7d1a
📒 Files selected for processing (11)
docs/ramalama-chat.1.mddocs/ramalama-run.1.mdramalama/arg_types.pyramalama/chat.pyramalama/chat_providers/openai.pyramalama/cli.pyramalama/plugins/runtimes/inference/common.pytest/e2e/test_run.pytest/unit/providers/test_openai_provider.pytest/unit/test_cli_args.pytest/unit/test_file_loader_integration.py
| Attaches the given file to the initial request. | ||
| Can be specified multiple times to attach multiple files. |
There was a problem hiding this comment.
Tighten wording for grammatical completeness.
Line 27 is a fragment. Consider: “It can be specified multiple times to attach multiple files.”
🧰 Tools
🪛 LanguageTool
[style] ~26-~26: To form a complete sentence, be sure to include a subject.
Context: ... the given file to the initial request. Can be specified multiple times to attach m...
(MISSING_IT_THERE)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/ramalama-chat.1.md` around lines 26 - 27, Update the second sentence in
the docs entry so the fragment becomes a complete sentence: replace "Can be
specified multiple times to attach multiple files." with "It can be specified
multiple times to attach multiple files." so the two-line description reads as
two grammatically complete sentences.
| Attaches the given file to the initial request. | ||
| Can be specified multiple times to attach multiple files. |
There was a problem hiding this comment.
Use a complete sentence in the option description.
Line 40 reads as a fragment. Suggest: “It can be specified multiple times to attach multiple files.”
🧰 Tools
🪛 LanguageTool
[style] ~39-~39: To form a complete sentence, be sure to include a subject.
Context: ... the given file to the initial request. Can be specified multiple times to attach m...
(MISSING_IT_THERE)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/ramalama-run.1.md` around lines 39 - 40, Replace the fragment "Can be
specified multiple times to attach multiple files." with a complete sentence for
the option description that follows "Attaches the given file to the initial
request.", e.g. "It can be specified multiple times to attach multiple files."
Update the option description text where the option's help string appears (the
line containing "Can be specified multiple times to attach multiple files.") so
the two sentences read as complete, grammatically correct sentences.
| mock_urlopen.return_value = mock_response | ||
|
|
||
| with tempfile.NamedTemporaryFile(suffix=".jpg") as tmp_file: | ||
| with open(tmp_file.name, "wb") as f: |
There was a problem hiding this comment.
again why open the same file twice?
There was a problem hiding this comment.
To ensure the contents is written to dish before it is read. Admittedly I forgot NamedTemporaryFile is a file-like itself and not a data descriptor. You could use flush() to get the same effect, although I do like the clean seperation of when file writes are done gotten by using a with.
There was a problem hiding this comment.
Does that work on windows? Generally can't open the same file twice but I think it works from within the same process.
IIRC this is already done elsewhere, something along the lines of:
from ramalama.compat import NamedTemporaryFile
with NamedTemporaryFile(delete_on_close=False) as tmp_file:
tmp_file.write("foobar")
tmp_file.close()
# Do stuff that reads the file
(the compat version is necessary to work on python 3.10 with does not have delete_on_close)
|
This PR requires a rebase. |
0ecdc8b to
8aade94
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/ramalama-chat.1.md (1)
75-97:⚠️ Potential issue | 🟡 MinorDocument
/attachin interactive commands forramalama chat.The new option is documented, but the interactive
/attachcommand is not listed, which can hide core functionality from users.📘 Suggested doc addition
#### **/clear** Clear the conversation history without exiting the chat session. This resets the context and allows starting a fresh conversation without restarting the container or connection. A confirmation message will be displayed when the history is cleared. +#### **/attach** *file-path* +Attach a file to the next request. This command can be used multiple times before sending a prompt. + #### **/bye**, **exit** Exit the chat session and close the connection.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/ramalama-chat.1.md` around lines 75 - 97, The interactive command list under INTERACTIVE COMMANDS for ramalama chat is missing documentation for the /attach command; add an entry for "/attach" under INTERACTIVE COMMANDS that shows the command syntax (e.g., /attach <path|url>), a brief description of what it does (attach/upload a file to the current conversation/context), note case-insensitivity and any prerequisites or flags required to enable it, and include a short example of usage so users can discover the feature.docs/ramalama-run.1.md (1)
286-307:⚠️ Potential issue | 🟡 MinorDocument
/attachin interactive commands forramalama run.This man page documents
--attach, but the interactive/attachcommand is not listed in## INTERACTIVE COMMANDS, which leaves the new interactive workflow undiscoverable.📘 Suggested doc addition
#### **/clear** Clear the conversation history without exiting the chat session. This resets the context and allows starting a fresh conversation without restarting the container or connection. A confirmation message will be displayed when the history is cleared. +#### **/attach** *file-path* +Attach a file to the next request. This command can be used multiple times before sending a prompt. + #### **/bye**, **exit** Exit the chat session and close the connection.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/ramalama-run.1.md` around lines 286 - 307, Add a new interactive command entry for "/attach" under the INTERACTIVE COMMANDS section to document the interactive attach workflow missing from the man page: describe the command syntax (e.g., "/attach [TARGET]"), what it does (attach the running session to a container/session started with --attach), behavior/limitations (case-insensitivity, required flags like --attach or --mcp if applicable), and the expected response or confirmation shown to the user; reference the existing "--attach" option in the description and ensure the entry matches the style of other commands like "/tool" and "/clear".
♻️ Duplicate comments (2)
docs/ramalama-chat.1.md (1)
26-27:⚠️ Potential issue | 🟡 MinorUse a complete sentence in the option description.
Please make the second sentence grammatically complete.
✏️ Suggested doc fix
-Can be specified multiple times to attach multiple files. +It can be specified multiple times to attach multiple files.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/ramalama-chat.1.md` around lines 26 - 27, The second sentence "Can be specified multiple times to attach multiple files." is a sentence fragment; update the option description containing "Attaches the given file to the initial request." and the following fragment so the second sentence is grammatically complete — e.g., change it to "It can be specified multiple times to attach multiple files." or merge into one sentence "Attaches the given file to the initial request and can be specified multiple times to attach multiple files." Make the edit where those exact phrases appear.docs/ramalama-run.1.md (1)
39-40:⚠️ Potential issue | 🟡 MinorUse a complete sentence in the option description.
Please change the fragment to a full sentence.
✏️ Suggested doc fix
-Can be specified multiple times to attach multiple files. +It can be specified multiple times to attach multiple files.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/ramalama-run.1.md` around lines 39 - 40, Replace the fragment "Can be specified multiple times to attach multiple files." with a full sentence such as "This option can be specified multiple times to attach multiple files." so the option description reads as complete sentences (e.g., keep "Attaches the given file to the initial request." and update the following line to the suggested full sentence).
🧹 Nitpick comments (1)
test/unit/test_cli_args.py (1)
144-149: Add value-level assertions forattachmentsround-trip.Current assertions only check attribute presence, so regressions in attachment parsing can still pass.
✅ Suggested test hardening
def test_chat_endpoint(chatargs): cli_args = args_to_cli_args(chatargs, 'chat', special_cases) args = parser.parse_args(cli_args) for field in ChatSubArgs.__dataclass_fields__: assert hasattr(args, field), f"Missing attribute: {field}" + + if chatargs.attachments: + assert args.attachments == chatargs.attachments + else: + assert args.attachments is None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/test_cli_args.py` around lines 144 - 149, The test_chat_endpoint currently only asserts attribute presence; add a concrete value-level assertion that the attachments round-trip is preserved by asserting args.attachments equals the expected value from the chatargs fixture (or the normalized representation produced by args_to_cli_args), e.g. after computing args = parser.parse_args(cli_args) add an assertion comparing args.attachments to chatargs.attachments (or to the CLI-serialized form if normalization occurs) so regressions in attachment parsing fail; update test_chat_endpoint and use the existing variables/functions (test_chat_endpoint, chatargs, args_to_cli_args, parser.parse_args, args, ChatSubArgs) to locate and implement the assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@docs/ramalama-chat.1.md`:
- Around line 75-97: The interactive command list under INTERACTIVE COMMANDS for
ramalama chat is missing documentation for the /attach command; add an entry for
"/attach" under INTERACTIVE COMMANDS that shows the command syntax (e.g.,
/attach <path|url>), a brief description of what it does (attach/upload a file
to the current conversation/context), note case-insensitivity and any
prerequisites or flags required to enable it, and include a short example of
usage so users can discover the feature.
In `@docs/ramalama-run.1.md`:
- Around line 286-307: Add a new interactive command entry for "/attach" under
the INTERACTIVE COMMANDS section to document the interactive attach workflow
missing from the man page: describe the command syntax (e.g., "/attach
[TARGET]"), what it does (attach the running session to a container/session
started with --attach), behavior/limitations (case-insensitivity, required flags
like --attach or --mcp if applicable), and the expected response or confirmation
shown to the user; reference the existing "--attach" option in the description
and ensure the entry matches the style of other commands like "/tool" and
"/clear".
---
Duplicate comments:
In `@docs/ramalama-chat.1.md`:
- Around line 26-27: The second sentence "Can be specified multiple times to
attach multiple files." is a sentence fragment; update the option description
containing "Attaches the given file to the initial request." and the following
fragment so the second sentence is grammatically complete — e.g., change it to
"It can be specified multiple times to attach multiple files." or merge into one
sentence "Attaches the given file to the initial request and can be specified
multiple times to attach multiple files." Make the edit where those exact
phrases appear.
In `@docs/ramalama-run.1.md`:
- Around line 39-40: Replace the fragment "Can be specified multiple times to
attach multiple files." with a full sentence such as "This option can be
specified multiple times to attach multiple files." so the option description
reads as complete sentences (e.g., keep "Attaches the given file to the initial
request." and update the following line to the suggested full sentence).
---
Nitpick comments:
In `@test/unit/test_cli_args.py`:
- Around line 144-149: The test_chat_endpoint currently only asserts attribute
presence; add a concrete value-level assertion that the attachments round-trip
is preserved by asserting args.attachments equals the expected value from the
chatargs fixture (or the normalized representation produced by
args_to_cli_args), e.g. after computing args = parser.parse_args(cli_args) add
an assertion comparing args.attachments to chatargs.attachments (or to the
CLI-serialized form if normalization occurs) so regressions in attachment
parsing fail; update test_chat_endpoint and use the existing variables/functions
(test_chat_endpoint, chatargs, args_to_cli_args, parser.parse_args, args,
ChatSubArgs) to locate and implement the assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e5560d19-5038-41d0-bdbd-466eface083b
📒 Files selected for processing (11)
docs/ramalama-chat.1.mddocs/ramalama-run.1.mdramalama/arg_types.pyramalama/chat.pyramalama/chat_providers/openai.pyramalama/cli.pyramalama/plugins/runtimes/inference/common.pytest/e2e/test_run.pytest/unit/providers/test_openai_provider.pytest/unit/test_cli_args.pytest/unit/test_file_loader_integration.py
🚧 Files skipped from review as they are similar to previous changes (7)
- ramalama/arg_types.py
- ramalama/plugins/runtimes/inference/common.py
- test/unit/providers/test_openai_provider.py
- test/e2e/test_run.py
- ramalama/chat_providers/openai.py
- test/unit/test_file_loader_integration.py
- ramalama/chat.py
|
Question: is |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
test/unit/test_file_loader_integration.py (1)
27-30:⚠️ Potential issue | 🟠 MajorUse the repo’s temp-file compatibility pattern consistently in these blocks.
These tests still reopen the same temp file path while the original handle is open, without
delete_on_close=False. This can be flaky across Python/OS combinations (notably Windows).Suggested pattern to apply in these blocks
-with NamedTemporaryFile(suffix=".txt") as tmp_file: - with open(tmp_file.name, "w") as f: - f.write("This is test content for chat input") +with NamedTemporaryFile(suffix=".txt", delete_on_close=False) as tmp_file: + tmp_file.write(b"This is test content for chat input") + tmp_file.close()Please verify all occurrences with:
#!/bin/bash # Find NamedTemporaryFile blocks and check for reopen-by-path patterns. rg -n "with NamedTemporaryFile\\(" test/unit/test_file_loader_integration.py -A6 -B1 rg -n "with open\\(tmp_file\\.name" test/unit/test_file_loader_integration.py -A2 -B1Expected result: every case requiring path-based reuse should use
delete_on_close=Falseand close/write viatmp_filebefore reading via path.
Based on learnings: "In this repo’s Python 3.10+ compatibility layer, always importNamedTemporaryFilefromramalama.compat... passdelete_on_close=Falsewhen the file must remain accessible by path within thewithblock."Also applies to: 55-58, 209-211, 236-239, 323-325, 344-346
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/test_file_loader_integration.py` around lines 27 - 30, The test reopens the same temp file path while the original NamedTemporaryFile handle is still open (using with NamedTemporaryFile(...) as tmp_file and then open(tmp_file.name,...)), which is flaky on Windows; update each occurrence (e.g., the tmp_file/tmp_file.name blocks at lines referenced) to use the repo pattern: import NamedTemporaryFile from ramalama.compat, create it with delete_on_close=False, write/flush/close via the tmp_file handle before reopening by path, and ensure any reopen uses tmp_file.name only after tmp_file is closed so the file is reliably accessible by path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ramalama/chat.py`:
- Around line 546-550: The attachment loading can raise and leave
self.conversation_history partially mutated; to fix, instantiate
OpanAIChatAPIMessageBuilder and for each attachment call builder.load inside a
try/except, collecting loaded messages into a temporary list (e.g., temp_msgs)
and only extend self.conversation_history with temp_msgs on success; on
exception log the error (or record it) and skip that attachment, and ensure
self.attachments is cleared after processing (use finally or clear at end) so a
failed load doesn't abort default() leaving inconsistent state; reference
OpanAIChatAPIMessageBuilder, builder.load, self.conversation_history, and
self.attachments.
---
Duplicate comments:
In `@test/unit/test_file_loader_integration.py`:
- Around line 27-30: The test reopens the same temp file path while the original
NamedTemporaryFile handle is still open (using with NamedTemporaryFile(...) as
tmp_file and then open(tmp_file.name,...)), which is flaky on Windows; update
each occurrence (e.g., the tmp_file/tmp_file.name blocks at lines referenced) to
use the repo pattern: import NamedTemporaryFile from ramalama.compat, create it
with delete_on_close=False, write/flush/close via the tmp_file handle before
reopening by path, and ensure any reopen uses tmp_file.name only after tmp_file
is closed so the file is reliably accessible by path.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 58ecf316-960d-487d-b2ba-8ef75efaaf42
📒 Files selected for processing (4)
ramalama/chat.pyramalama/chat_providers/openai.pytest/e2e/test_run.pytest/unit/test_file_loader_integration.py
🚧 Files skipped from review as they are similar to previous changes (2)
- test/e2e/test_run.py
- ramalama/chat_providers/openai.py
| if self.attachments: | ||
| builder = OpanAIChatAPIMessageBuilder() | ||
| for attachment in self.attachments: | ||
| self.conversation_history.extend(builder.load(attachment)) | ||
| self.attachments = [] |
There was a problem hiding this comment.
Handle attachment load failures to avoid request-time crashes.
At Line [549], builder.load(attachment) can raise (e.g., file deleted after /attach or between startup and first prompt). Right now that aborts default() and can leave partially-mutated history.
Suggested hardening
if self.attachments:
builder = OpanAIChatAPIMessageBuilder()
- for attachment in self.attachments:
- self.conversation_history.extend(builder.load(attachment))
- self.attachments = []
+ loaded_messages: list[ChatMessageType] = []
+ for attachment in self.attachments:
+ try:
+ loaded_messages.extend(builder.load(attachment))
+ except (OSError, ValueError) as e:
+ perror(f"Failed to load attachment {attachment}: {e}")
+ self.conversation_history.extend(loaded_messages)
+ self.attachments = []🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ramalama/chat.py` around lines 546 - 550, The attachment loading can raise
and leave self.conversation_history partially mutated; to fix, instantiate
OpanAIChatAPIMessageBuilder and for each attachment call builder.load inside a
try/except, collecting loaded messages into a temporary list (e.g., temp_msgs)
and only extend self.conversation_history with temp_msgs on success; on
exception log the error (or record it) and skip that attachment, and ensure
self.attachments is cleared after processing (use finally or clear at end) so a
failed load doesn't abort default() leaving inconsistent state; reference
OpanAIChatAPIMessageBuilder, builder.load, self.conversation_history, and
self.attachments.
e5997b2 to
07b922f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/ramalama-chat.1.md`:
- Around line 25-27: Update the --attach option documentation to mention the
interactive /attach command as an alternative entry point: in the block
describing **--attach**=**file-path** (and its "Can be specified multiple times"
note) add a short sentence like "Files can also be attached during an
interactive chat session using the /attach command" or a cross-reference to the
interactive command section so users know both the initial-request flag and the
interactive `/attach` path are available.
In `@test/unit/test_file_loader_integration.py`:
- Around line 27-29: Change each NamedTemporaryFile usage that reopens
tmp_file.name to create the temp file with delete=False and explicitly close it
before calling open(tmp_file.name, ...) — e.g., replace
NamedTemporaryFile(suffix=".txt") as tmp_file with
NamedTemporaryFile(suffix=".txt", delete=False) as tmp_file: tmp_path =
tmp_file.name; tmp_file.close(); then use open(tmp_path, "w") to write and later
remove the file when done. Apply this pattern to the blocks that reference
tmp_file.name (the occurrences around the tmp_file variable at lines showing
tmp_file usage).
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: bdab4cd2-3fbe-4b06-8044-e8134ab886fb
📒 Files selected for processing (10)
docs/ramalama-chat.1.mdramalama/arg_types.pyramalama/chat.pyramalama/chat_providers/openai.pyramalama/cli.pyramalama/plugins/runtimes/inference/common.pytest/e2e/test_run.pytest/unit/providers/test_openai_provider.pytest/unit/test_cli_args.pytest/unit/test_file_loader_integration.py
✅ Files skipped from review due to trivial changes (1)
- ramalama/plugins/runtimes/inference/common.py
🚧 Files skipped from review as they are similar to previous changes (5)
- ramalama/arg_types.py
- test/unit/providers/test_openai_provider.py
- test/unit/test_cli_args.py
- ramalama/chat.py
- test/e2e/test_run.py
| #### **--attach**=**file-path** | ||
| Attaches the given file to the initial request. | ||
| Can be specified multiple times to attach multiple files. |
There was a problem hiding this comment.
Document the interactive /attach path in this option block.
The feature includes attaching files during interactive chat, but this section only describes initial-request behavior. Please add a short note here (or cross-reference the interactive command section) so users discover both entry points.
📝 Suggested doc tweak
#### **--attach**=**file-path**
Attaches the given file to the initial request.
Can be specified multiple times to attach multiple files.
+In interactive mode, use `/attach <file-path>` to attach files to the next request.🧰 Tools
🪛 LanguageTool
[style] ~26-~26: To form a complete sentence, be sure to include a subject.
Context: ... the given file to the initial request. Can be specified multiple times to attach m...
(MISSING_IT_THERE)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/ramalama-chat.1.md` around lines 25 - 27, Update the --attach option
documentation to mention the interactive /attach command as an alternative entry
point: in the block describing **--attach**=**file-path** (and its "Can be
specified multiple times" note) add a short sentence like "Files can also be
attached during an interactive chat session using the /attach command" or a
cross-reference to the interactive command section so users know both the
initial-request flag and the interactive `/attach` path are available.
|
Question: why doesn't |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
docs/options/attach.md (2)
7-7: Optional: Add subject for grammatical completeness.The static analysis tool suggests adding a subject (e.g., "It can be specified...") for grammatical completeness, though the current phrasing is acceptable in technical documentation.
✍️ Optional style improvement
-Can be specified multiple times to attach multiple files. +It can be specified multiple times to attach multiple files.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/options/attach.md` at line 7, Replace the sentence "Can be specified multiple times to attach multiple files." with a grammatically complete version such as "It can be specified multiple times to attach multiple files." — update the phrase in the docs/attach documentation where that exact sentence appears to include the subject "It" for completeness.
6-7: Consider mentioning the/attachinteractive subcommand.The documentation accurately describes the
--attachCLI option, but the PR description states: "In interactive mode, files can be attached to the next request using the /attach subcommand." Consider adding a note about this interactive feature for completeness, as users might benefit from knowing both methods.📝 Suggested enhancement to mention interactive mode
Attaches the given file to the initial request. -Can be specified multiple times to attach multiple files. +Can be specified multiple times to attach multiple files. +In interactive mode, use the `/attach` subcommand to attach files to subsequent requests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/options/attach.md` around lines 6 - 7, The docs currently explain the CLI --attach option but omit the interactive-mode counterpart; update the attach documentation to briefly mention the interactive /attach subcommand (referencing the CLI flag --attach and the interactive command /attach) and add one short sentence explaining that in interactive sessions files can be attached to the next request using /attach, including any usage example or note that it can be invoked multiple times similar to --attach.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@docs/options/attach.md`:
- Line 7: Replace the sentence "Can be specified multiple times to attach
multiple files." with a grammatically complete version such as "It can be
specified multiple times to attach multiple files." — update the phrase in the
docs/attach documentation where that exact sentence appears to include the
subject "It" for completeness.
- Around line 6-7: The docs currently explain the CLI --attach option but omit
the interactive-mode counterpart; update the attach documentation to briefly
mention the interactive /attach subcommand (referencing the CLI flag --attach
and the interactive command /attach) and add one short sentence explaining that
in interactive sessions files can be attached to the next request using /attach,
including any usage example or note that it can be invoked multiple times
similar to --attach.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6615a036-8e8d-430e-b3fb-67bb6002879a
📒 Files selected for processing (2)
docs/options/attach.mddocs/ramalama-run.1.md.in
✅ Files skipped from review due to trivial changes (1)
- docs/ramalama-run.1.md.in
Added a new repeatable `--attach file` argument to both `ramalama run` and `ramalama chat`. This argument attaches the given file to the initial request to the server. Additionally, in interactive mode, you can attach files to your next request using the `/attach` subcommand. This is useful for a couple of things: - Analyzing log files: `ramalama run MODEL --attach server.log What errors occurred at 1pm? - Analyzing pictures on the command line: `ramalama run MODEL --attach bird.jpg What kind of bird is this?` - Doing an AI review before committing: `ramalama run MODEL $(git diff --name-only | sed 's/^/--attach /') Do a review of these changes: $(git diff)` The existing file serialization options inside OpanAIChatAPIMessageBuilder and serialize_part were leveraged to implement this feature. Signed-off-by: Christopher Chianelli <christopher@timefold.ai>
Signed-off-by: Christopher Chianelli <christopher@timefold.ai>
Signed-off-by: Christopher Chianelli <christopher@timefold.ai>
703d8a4 to
674f51c
Compare
GitHub Actions
Inactive
|
We only use the .in format, if a man page is using a shared option with other man pages. If you see an option that is in the ramana-chat man page, then we should move it to the .in format. |
|
A friendly reminder that this PR had no activity for 30 days. |
3 participants
Added a new repeatable
--attach fileargument to bothramalama runandramalama chat. This argument attaches the given file to the initial request to the server. Additionally, in interactive mode, you can attach files to your next request using the/attachsubcommand.This is useful for a couple of things:
ramalama run MODEL --attach bird.jpg What kind of bird is this?ramalama run MODEL $(git diff --name-only | sed 's/^/--attach /') Do a review of these changes: $(git diff)The existing file serialization options inside OpanAIChatAPIMessageBuilder and serialize_part were leveraged to implement this feature.