fix: Replace buggy append_metadata_to_message implementation#308
Closed
ezyang wants to merge 1 commit intogh/ezyang/229/basefrom
Closed
fix: Replace buggy append_metadata_to_message implementation#308ezyang wants to merge 1 commit intogh/ezyang/229/basefrom
ezyang wants to merge 1 commit intogh/ezyang/229/basefrom
Conversation
This was referenced Jun 4, 2025
dmerkert
pushed a commit
to dmerkert/codemcp
that referenced
this pull request
Jun 4, 2025
The implementation of append_metadata_to_message is a bit buggy and it is failing tests FAILED tests/test_git_message.py::**TestGitMessageHandling::test_append_to_message_with_trailing_newlines** - AssertionError: 'feat: Add feature\n\nDescription\n\ncodemcp-id: abc-123\n\n' != 'feat: Add feature\n\nDescri... FAILED tests/test_git_message.py::**TestGitMessageHandling::test_append_to_message_with_double_trailing_newlines** - AssertionError: 'feat: Add feature\n\nDescription\n\ncodemcp-id: abc-123\n\n\n' != 'feat: Add feature\n\nDesc... Here is a better impl attached, please slot it in. ```git-revs 940b906 (Base revision) a04e927 Add git_parse_message module with improved trailer parsing implementation 8ec84ca Replace buggy append_metadata_to_message implementation with improved version 6249c20 Remove subprocess import from git_message.py since we're no longer using git interpret-trailers 513ae0f Fix interpret_trailers to handle trailing newlines properly based on original message structure 4a9f30d Fix trailer insertion logic to avoid adding extra newlines by using correct spacing 7e993e1 Rewrite interpret_trailers to follow the expected behavior from tests 927a645 Fix spacing for messages without body - they need single newline not double 7d00495 Fix empty message case to match expected format f578782 Fix empty message case by handling it as a special case with correct spacing HEAD Skip the trailing newline logic for empty messages since we handle it specially ``` codemcp-id: 2-fix-replace-buggy-append-metadata-to-message-imple ghstack-source-id: e25b62e Pull-Request-resolved: ezyang#308
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.
Stack from ghstack (oldest at bottom):
The implementation of append_metadata_to_message is a bit buggy and it is failing tests
FAILED tests/test_git_message.py::TestGitMessageHandling::test_append_to_message_with_trailing_newlines - AssertionError: 'feat: Add feature\n\nDescription\n\ncodemcp-id: abc-123\n\n' != 'feat: Add feature\n\nDescri...
FAILED tests/test_git_message.py::TestGitMessageHandling::test_append_to_message_with_double_trailing_newlines - AssertionError: 'feat: Add feature\n\nDescription\n\ncodemcp-id: abc-123\n\n\n' != 'feat: Add feature\n\nDesc...
Here is a better impl attached, please slot it in.
codemcp-id: 2-fix-replace-buggy-append-metadata-to-message-imple