fix commit message invalid utf8#4009
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses #3993 by centralizing commit message sanitization at the point where the message is persisted, preventing PostgreSQL UTF-8 transaction failures caused by invalid byte sequences and ensuring the stored commit message stays within the DB column limit.
Changes:
- Sanitize persisted commit messages by stripping NUL bytes, replacing invalid UTF-8 sequences, and capping length by runes.
- Remove Git-client-side truncation logic and its tests, shifting responsibility to the persistence layer.
- Add targeted unit tests for UTF-8 sanitization and rune-based capping.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| services/tasks/TaskRunner_logging.go | Sanitizes commit messages before persisting task updates. |
| pkg/conv/conv.go | Introduces UTF-8 sanitization + rune-capping helper for commit messages. |
| pkg/conv/conv_test.go | Adds tests covering valid input, rune caps, invalid bytes, and NUL stripping. |
| db_lib/GoGitClient.go | Stops truncating commit messages at fetch time (now returns full message). |
| db_lib/CmdGitClient.go | Removes commit message truncation at fetch time. |
| db_lib/commit_message.go | Deletes legacy rune-truncation helper (superseded by conv sanitization). |
| db_lib/commit_message_test.go | Removes tests for deleted legacy truncation helper. |
3b0a367 to
0b52f11
Compare
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
💤 Files with no reviewable changes (3)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughThis PR adds UTF-8-safe commit message truncation, uses it for task commit-message storage and logging, and returns the raw git commit message in one retrieval path. ChangesCommit message sanitization
Estimated code review effort: 2 (Simple) | ~12 minutes Sequence Diagram(s)sequenceDiagram
participant GoGitClient
participant conv
participant TaskRunner
participant LocalExecutor
participant Task
GoGitClient->>conv: TruncateValidUTF8(msg)
conv-->>GoGitClient: sanitized log message
TaskRunner->>conv: TruncateValidUTF8(message)
conv-->>TaskRunner: sanitized message
TaskRunner->>Task: store CommitMessage
LocalExecutor->>conv: TruncateValidUTF8(message)
conv-->>LocalExecutor: sanitized message
LocalExecutor->>Task: store CommitMessage
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Tools execution failed with the following error: Failed to run tools: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error) Comment |
0b52f11 to
7e8537f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/conv/conv.go (1)
10-23: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider parameterizing the rune limit.
commitMessageMaxRunesand the doc comment tie this general-purposepkg/convutility to the commit-message domain specifically. If other callers ever need a different length cap, this hardcoded 100 will need to be duplicated or the function forked. Consider exposing the limit as a parameter (e.g.,TruncateValidUTF8(s string, maxRunes int)) with a commit-message-specific wrapper/constant kept in the calling package.🤖 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 `@pkg/conv/conv.go` around lines 10 - 23, The UTF-8 truncation helper is hardcoded to the commit-message rune limit, which makes the generic utility inflexible. Update TruncateValidUTF8 to accept a max-rune parameter and use that throughout the truncation logic, then move the commit-message-specific constant and any wrapper into the calling package so pkg/conv remains reusable. Keep the existing behavior for sanitizing invalid UTF-8 and NUL bytes, but remove the commit-message-specific coupling from the function name/comment and internal limit.
🤖 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 `@db_lib/GoGitClient.go`:
- Around line 234-239: The commit message returned by GetLastCommitMessage is
being passed through unsanitized into the local executor flow, so update
GoGitClient.GetLastCommitMessage to sanitize the message before returning it,
using the same truncation/UTF-8-safe handling as TaskRunner.SetCommit or a
shared helper. Ensure the value that reaches local_executor.go and then
Task.CommitMessage/taskDetails is already valid UTF-8, and keep the logging
consistent with the sanitized message.
---
Nitpick comments:
In `@pkg/conv/conv.go`:
- Around line 10-23: The UTF-8 truncation helper is hardcoded to the
commit-message rune limit, which makes the generic utility inflexible. Update
TruncateValidUTF8 to accept a max-rune parameter and use that throughout the
truncation logic, then move the commit-message-specific constant and any wrapper
into the calling package so pkg/conv remains reusable. Keep the existing
behavior for sanitizing invalid UTF-8 and NUL bytes, but remove the
commit-message-specific coupling from the function name/comment and internal
limit.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f9352231-a5b1-46b7-8ecf-f76a320fad6e
📒 Files selected for processing (7)
db_lib/CmdGitClient.godb_lib/GoGitClient.godb_lib/commit_message.godb_lib/commit_message_test.gopkg/conv/conv.gopkg/conv/conv_test.goservices/tasks/TaskRunner_logging.go
💤 Files with no reviewable changes (3)
- db_lib/CmdGitClient.go
- db_lib/commit_message_test.go
- db_lib/commit_message.go
7e8537f to
98ece29
Compare
|
@codex review |
|
@cursoragent review |
|
Note Unit test generation is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
❌ Failed to create PR with unit tests: AGENT_CHAT: Failed to open pull request |
6e96ad3 to
0b00923
Compare
fix issue #3993
Summary by CodeRabbit
New Features
Bug Fixes