feat: work on adding URI support to messages (for URLs and MCP resources) #529
Conversation
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
Contributor
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed everything up to 47ad7fb in 1 minute and 56 seconds. Click for details.
- Reviewed
92lines of code in3files - Skipped
0files when reviewing. - Skipped posting
4draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. gptme/logmanager.py:467
- Draft comment:
Using URIPath(f) here ensures consistent URI/file handling. Make sure URIPath properly supports both local paths and URI strings. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is asking the PR author to ensure that a specific functionality is supported, which violates the rule against asking the author to ensure behavior is intended or tested. It does not provide a specific suggestion or point out a specific issue.
2. gptme/message.py:168
- Draft comment:
Consistently parse files with URIPath in from_toml and toml_to_msgs. Verify that URIPath conversion doesn’t break existing file path behavior. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
3. gptme/util/context.py:131
- Draft comment:
Converting file paths with URIPath(str(f)) may be redundant if f is already a Path. Consider passing f directly if URIPath supports Path input. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% Without seeing the URIPath implementation, I can't be certain whether passing a Path object directly would work. The explicit str() conversion suggests the author may have had a reason for ensuring string input. The comment is speculative ("may be redundant", "if URIPath supports") rather than definitive. This kind of speculative suggestion violates our rules. I could be wrong about the URIPath implementation - maybe it's documented somewhere that it accepts Path objects directly. The str() conversion could be an oversight rather than intentional. Without seeing the URIPath implementation or documentation, we can't be confident that this suggestion is correct. The speculative nature of the comment makes it less useful. Delete the comment since it's speculative and we don't have strong evidence that the suggestion would be an improvement.
4. gptme/util/context.py:216
- Draft comment:
Typo: 'pah' should be 'path' in the comment. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% While the comment is technically correct in pointing out a typo, it's just a typo in a comment, not in actual code. Comments are documentation and while they should be correct, this is an extremely minor issue that doesn't affect functionality. The rules state to only keep comments that require clear code changes and to not make obvious or unimportant comments. The typo could theoretically make the code slightly harder to understand for future readers. And since it's in changed code (new comment), it is related to the changes. While true, this is still an extremely minor documentation issue that doesn't meaningfully impact code understanding - the meaning is still clear from context. The rules explicitly say not to make obvious or unimportant comments. This comment should be deleted as it points out an extremely minor documentation issue that doesn't require meaningful code changes.
Workflow ID: wflow_c3dnwTQ9uiISrZk8
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
| files: Counter[Path] = Counter() | ||
| for msg in msgs: | ||
| for f in msg.files: | ||
| # If pah is URLPath |
Contributor
There was a problem hiding this comment.
Typo in comment: 'If pah is URLPath' should be 'If path is URIPath'. Also, consider handling URIPath instances separately to avoid calling resolve() on non-local URIs.
Suggested change
| # If pah is URLPath | |
| # If path is URIPath |
Contributor
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 4a38a8b in 1 minute and 40 seconds. Click for details.
- Reviewed
103lines of code in1files - Skipped
0files when reviewing. - Skipped posting
5draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. gptme/util/uri_path.py:6
- Draft comment:
URI_PATTERN uses a simple regex for URI schemes. Ensure it covers all valid schemes expected in practice. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. gptme/util/uri_path.py:22
- Draft comment:
Creating a Path with an empty string for URIs may lead to unexpected behavior in path operations (e.g. joining paths). Consider overriding join-related methods or documenting this limitation. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50% The comment raises a valid technical concern - path joining with an empty string base could cause issues. However, looking at the class's purpose and implementation, it seems URIs and filesystem paths are meant to be handled separately. The _is_uri flag is used to branch behavior, and URIs likely shouldn't be joined with filesystem paths anyway. The empty string base is an implementation detail that's hidden behind the overridden methods. I might be underestimating the impact of not handling path joining operations. Users of the class might reasonably expect to be able to join paths. While path joining could be an issue, the class appears to be designed to keep URI handling separate from filesystem paths. Adding join support would add complexity for potentially unused functionality. The comment should be deleted as it suggests addressing a theoretical issue without clear evidence that it's actually needed in practice.
3. gptme/util/uri_path.py:37
- Draft comment:
Multiple methods use 'getattr(self, "_is_uri", False)' even though _is_uri is always set in new. Using direct attribute access (self._is_uri) could improve clarity and slight performance. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
4. gptme/util/uri_path.py:61
- Draft comment:
Filesystem methods (exists, is_file, is_dir) return fixed values when handling URIs. Confirm that this behavior is intentionally abstracting non-filesystem resources. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
5. gptme/util/uri_path.py:92
- Draft comment:
The absolute() method relies on super().absolute(), which might not be consistently defined across all pathlib implementations. Verify this behavior for cross-platform consistency. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
Workflow ID: wflow_emRmBYdotqBdCTNY
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Working towards: #516
Might also attempt: #468
Important
Adds
URIPathclass to support URI handling in messages, updating relevant functions and classes to accommodate URI and file path integration.URIPathclass inuri_path.pyto handle URI strings likehttp://andmemo://.Messageclass inmessage.pyto useURIPathforfilesattribute, allowing URIs alongside file paths._gen_read_jsonl()inlogmanager.pyto useURIPathfor file handling.append_file_content()andget_mentioned_files()incontext.pyto handleURIPathinstances.URIPathinlogmanager.py,message.py, andcontext.py.This description was created by
for 4a38a8b. You can customize this summary. It will automatically update as commits are pushed.